Vscode-extension-for-zowe: tests failing randomly

[VitGottwald]

jest-stare is causing test failures because it is trying to create existing directories. I opened an issue against it and suggest removing jest-stare. Once the issue is fixed and someone wants nice html reports, we can add it back.

[ghost]

Is this the same issue as #710? If so the issue is not caused by jest-stare, but rather by fs mocks that our tests fail to reset, and it should be fixed by #759.

[VitGottwald]

Not sure @tjohnsonBCM . When I replace the custom code in https://github.com/dkelosky/jest-stare/blob/740dde23253f3056ef48bcf8fb1d220a5f097441/src/utils/IO.ts#L73 with mkdirp it works.

[ghost]

Not sure @tjohnsonBCM . When I replace the custom code in https://github.com/dkelosky/jest-stare/blob/740dde23253f3056ef48bcf8fb1d220a5f097441/src/utils/IO.ts#L73 with mkdirp it works.

I investigated this error in depth and don't think jest-stare is doing anything wrong. It checks if a directory exists before creating it, which mimics the behavior of mkdirp.

The error message is puzzling:
EISDIR: illegal operation on a directory, mkdir '/'
The root directory has to already exist, so why does jest-stare think it doesn't exist and try to create it?

The only answer I could find is that our tests override fs.existsSync with a mock that always returns False. This mock is not being reset properly by some of our unit tests.

The error only manifests itself when jest-stare runs in the same process as the unit tests (with the option --runInBand). If it is running in a different process then it doesn't matter whether the fs mock is reset.

This is why the npm run test:unit command succeeds, because it doesn't use --runInBand. The problem has only happened in the VSCode launch task for unit tests, which does use --runInBand to support debugging.

When I modified our tests to properly reset the mock, this error went away and jest-stare worked fine with the --runInBand option.

To make sure our mocks get reset properly in the future, I suggested in https://github.com/zowe/vscode-extension-for-zowe/issues/751#issuecomment-621320712 that when refactoring tests we change them to use jest.spyOn instead of jest.mock, so that restoreAllMocks can reset all the mocks easily in one operation.

[dkelosky]

Can you add jest.mock("fs") and see if that fixes the problem?

I think the way fs is altered via: Object.defineProperty(fs, "existsSync", {value: existsSync}); means that jest.mock("fs") should be added to the beginning of uss/ZoweUSSNode.unit.test.ts.

It looks like we had this for data sets unit tests.

I don't think jest-stare is failing tests. As @tjohnsonBCM observed, it looks like a mocking issue. Presumably we could also alter mkdirp in such a way to make it fail as well.

[katelynienaber]

I agree with @tjohnsonBCM's analysis, but I tried using spyOn in the mocks and it isn't letting me spyOn static functions or functions defined in interfaces (like vscode's functions). We could mock the entire class but after discussing with @VitGottwald I decided to try something easier first.

Please take a look at file /__tests__/__unit__/uss/ZoweUSSNode.unit.test.ts on PR #709: link to file

@tjohnsonBCM do you think the setup there would fix the fs mocking issue?

[VitGottwald]

It checks if a directory exists before creating it, which mimics the behavior of mkdirp

Not really, the loop here https://github.com/dkelosky/jest-stare/blob/740dde23253f3056ef48bcf8fb1d220a5f097441/src/utils/IO.ts#L83 goes in and tries to create a directory at every level of the new path.

When I replaced the mkdirsSync implementation with a call to mkdirp the problems went away.

[VitGottwald]

@dkelosky I could investigate in more details why it worked with mkdirp and not with the current implementation in jest-stere. But that will divert energy from getting our refactor done.

Without jest-stare I can run jest in watch mode and get consistent feedback and that is what we need.

[VitGottwald]

@tjohnsonBCM if you want to investigate this issue further it would be interesting to see why removing jest-stare "fixes" the issues.

[dkelosky]

@VitGottwald Did you have any luck with adding jest.mock("fs") to the test file? I think that's the problem

[VitGottwald]

Yes @dkelosky, there were a few more changes that were needed, but the key is jest.mock("fs") .

[dkelosky]

Very cool 😄 !

Can you link the other changes? Thanks for sharing Jest doc!

[ghost]

Please take a look at file /__tests__/__unit__/uss/ZoweUSSNode.unit.test.ts on PR #709: link to file

@tjohnsonBCM do you think the setup there would fix the fs mocking issue?

@katelynienaber My concerns about fs mocks not getting restored have been alleviated by this discovery from @VitGottwald: https://github.com/zowe/vscode-extension-for-zowe/pull/775#issuecomment-623952979

Seems like using spyOn may not be necessary, and we should be good as long as we call jest.mock("{builtinNodejsModuleName}") at the top of our unit test files for any built-in modules we mock (fs, path, etc).

[VitGottwald]

@dkelosky this test was failing because isbinaryfile wants a real fs module.

[VitGottwald]

Now waiting for jenkins to work 😞

[dkelosky]

Got it. Thanks for the update - just looking to capture if there were more mocking learnings to capture

[VitGottwald]

You identified the culprit. Some tests (integration) are using real 'fs' module. Unit tests are using a mocked one. But some of the unit tests were not calling jest.mock so they got the real module and modified it. We should never modify the real module.

Now the question is what other core module are we modifying?

[VitGottwald]

Many thanks to @dkelosky and @tjohnsonBCM for their hard work and patience with myself during this issue resolution, you are awesome guys! 💯

[dkelosky]

Thanks for working on it - this was a good collective discovery.

Perhaps jest-stare can be removed in a future effort if it's not serving any purpose.