Eslint check for mocking node core modules

[VitGottwald]

No description provided.

[codecov]

Codecov Report

Merging #786 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #786   +/-   ##
=======================================
  Coverage   91.23%   91.23%           
=======================================
  Files          47       47           
  Lines        5143     5143           
  Branches     1076     1076           
=======================================
  Hits         4692     4692           
  Misses        447      447           
  Partials        4        4           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b10640...12db28b. Read the comment docs.

[katelynienaber]

I want to mock these, but in the test PRs I am having trouble mocking node's standard module path.

Sometimes we need to use the standard behavior of path inside the test environment.

Maybe we could use something like this: https://www.npmjs.com/package/mock-fs

Or maybe there is a mock library someone has already made for path, somewhere on the interwebs?

[jellypuno]

Please see my comments

[jellypuno]

I am confused with the word plugin. For me, plugin is optional but somehow I feel like this is not optional.

[VitGottwald]

Eslint has several parts. Its core (the eslint package), plugins (which can provide linting rules) and a user configuration.

If someone wants to create a new rule for ESLint, they need to create an eslint plugin with the rule. That is what the eslint-plugin folder is.

To tell eslint I want it to use the plugin, the plugin has to be specified in eslint config https://github.com/zowe/vscode-extension-for-zowe/pull/786/files#diff-e4403a877d80de653400d88d85e4801aR4 . And to tell eslint to use a rule from the plugin for linting, it also has to be specified in the eslint config https://github.com/zowe/vscode-extension-for-zowe/pull/786/files#diff-e4403a877d80de653400d88d85e4801aR7.

[VitGottwald]

Does that answer your questions @jellypuno ?

[VitGottwald]

If the team was interested in more details, I could do a small presentation and explain how today's javascript/typescript tooling works under the hood.

[VitGottwald]

@katelynienaber I believe I answered your question in a standup. Because we are modifying the fs module, we have to make sure it is mocked. This eslint rule will make sure we call jest.mock('fs').

[phaumer]

Do we really want to run two linters? I would propose to switch everything over to eslint at once. I did some experimentation with it here: zowe/zowe-explorer-ftp-extension#3

I would also not add any .js files. As we are typescript project we should implement everything strongly typed in .ts files. Configurations such as .eslintrc are easier to maintain as yaml files.

[VitGottwald]

Good points @phaumer ! I would switch to ESlint without a blink of an eye. I actually suggested it about a month ago, but it has not been discussed yet and had no priority.

The goal of this PR is to prevent a very particular kind of error we discovered in our tests. I do not think its scope includes switching linters. For a short period of time I do not see a problem with having two of them.

[VitGottwald]

I guess I used .eslintrc.js because I am used to it. I have no problem switching to yaml. There might however be some quirks related to running prettier. In the last project I had a conditional logic in eslintrc to only run prettier on CI to avoid the problems I described in zowe/zowe-explorer-ftp-extension#3

[VitGottwald]

The .js question is also interesting one. ESlint's preferred way of providing custom linting rules is to provide an npm package with a plugin. They used to have the option to point to a directory with a rule, but they have long wanted to get rid of it and finally dropped it to have a clean architecture.
So we have to have a separate package for it.
If I write the plugin in typescript, I should define it as a typescript project and run a compile for it. Having a typescript project inside a typescript project causes troubles with compiler settings.
So the right thing to do is to create a workspace and make the ZOWE Explorer one package and the eslint plugin another package. Currently npm does not support workspaces (even though they plan it). So we would have to switch from npm to yarn.

I thought about all of this and decided that instead of boiling the ocean and introducing new concepts and additional tooling to he project, I would make it simple and go with javascript for the plugin. It is a lint tool, not production code.

What do you think we should do @phaumer ?

[phaumer]

Ok, I did not realize that. If this is just a tool that does not require constant maintenance and not production code then I agree to do it in just the most straightforward way. Sorry for chiming in without fully understanding what is going on.

I definitely like yaml files for listing the rules for our main code, but if that also causes problem then I understand.

[VitGottwald]

No worries @phaumer. All feedback is appreciated. It is my fault. I was so happy to have the solution to make sure we do not break tests again by modifying the fs module, that I rushed in with the PR. I should really have diligently described the solution and choices I made to everyone upfront.

[VitGottwald]

For this particular PR we can go with .yaml, I will update it. I think the discussion about how to run prettier will determine if we need to switch back to .js or can stay with yaml.

[zFernand0]

Thanks @VitGottwald
This will make sure that any new/refactored tests PRs do the right thing! 👍

[zFernand0]

Please keep in mind that the migration from tslint to eslint is still pending. and we will continue further discussion in zowe/zowe-explorer-ftp-extension#3

Also another reminder, let's aim to make eslint rule changes whenever there are a small amount of open PRs 😋