Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test infrastructure and babel #1686

Merged
merged 1 commit into from
Mar 22, 2020
Merged

Update test infrastructure and babel #1686

merged 1 commit into from
Mar 22, 2020

Conversation

adjerbetian
Copy link

I wanted to work on #1481 but the tests were failing locally. This PR fixes that.

Main change : babel configuration

The tests failed on master because the require function failed to load some package.

Error details
> cross-env BABEL_ENV=test NODE_PATH=./src nyc -s mocha -R dot --recursive -t 5s "tests/src"

internal/modules/cjs/loader.js:985
  throw err;
  ^

Error: Cannot find module 'core/importType'
Require stack:
- /home/djerbetian/code/eslint-plugin-import/tests/src/core/importType.js
- /home/djerbetian/code/eslint-plugin-import/node_modules/mocha/lib/mocha.js
- /home/djerbetian/code/eslint-plugin-import/node_modules/mocha/index.js
- /home/djerbetian/code/eslint-plugin-import/node_modules/mocha/bin/_mocha
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:982:15)
    at Function.Module._load (internal/modules/cjs/loader.js:864:27)
    at Module.require (internal/modules/cjs/loader.js:1044:19)
    at require (internal/modules/cjs/helpers.js:77:18)

The problem was from some dynamic require. I fixed the problem here: https://github.com/adjerbetian/eslint-plugin-import/commit/3e592e68edc551a2ef8065066470e17d7b695797.

Other change: mocha configuration

The current test configuration was spread between nyc (here) and mocha. I migrated this babel compiler configuration to .mocharc so it's easily reusable. See here: https://github.com/adjerbetian/eslint-plugin-import/commit/633bfeb81556600693b74a1e79c6716c09df2b1c.

Note: as I updated mocha so it supports .mocharc, it removed the lodash.isarray dependency that was assumed to be present in some tests, so I added it manually.

@coveralls
Copy link

coveralls commented Mar 15, 2020

Coverage Status

Coverage decreased (-1.9%) to 95.869% when pulling efb5f07 on adjerbetian:update-test-infrastructure into 1a3a128 on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Babel 7 and Mocha 4+ don’t support the versions of node we need to test on, so we effectively can’t ever upgrade to them.

It’d be great to fix the local test failures without updating any majors.

package.json Outdated
"@eslint/import-test-order-redirect-scoped": "file:./tests/files/order-redirect-scoped",
"@test-scope/some-module": "file:./tests/files/symlinked-module",
"@types/chai": "^4.2.11",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project isn’t in typescript, so these types aren’t useful.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb ok, I removed them. 👍

Usually, I still like to have them still as it improves the IDE autocompletion. But there are ways around it.

resolvers/webpack/.babelrc.js Outdated Show resolved Hide resolved
@adjerbetian
Copy link
Author

@ljharb Thank you for your answers!

Ok, I'll try to make the tests pass without touching mocha and babel.

Thanks again !

.gitignore Outdated
@@ -49,3 +49,6 @@ lib/
yarn.lock
package-lock.json
npm-shrinkwrap.json

# IDE
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can go in your global gitignore rather than in every repo you touch :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb True, but on some projects, I like to commit some IDE configuration. It makes it easier also to share some configuration with other people, and with myself when I switch computers 😅.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be done via dotfiles repos and similar :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb I'm not sure of what you mean by "dotfiles repo", but if you mean a configuration repository common to all projects (and team), that is a complementary option, but some configurations are still custom to the project and therefore belong in the project itself. I see you even have a file like this here: import.sublime-project.

But ok, I understand you really don't want to have this added to the .gitignore, I don't mind 😉. I removed it here: https://github.com/adjerbetian/eslint-plugin-import/commit/ba0d05ad1ae5f3e77ed741782a23031713221925

.mocharc.yml Outdated Show resolved Hide resolved
@adjerbetian adjerbetian requested a review from ljharb March 15, 2020 17:48
@adjerbetian
Copy link
Author

@ljharb I simplified my modification to only make my local tests pass. What do you think?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks much better!

.babelrc Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
package.json Outdated
"tests-only": "npm run mocha tests/src",
"test": "npm run tests-only",
"test-compiled": "npm run prepublish && NODE_PATH=./lib mocha --compilers js:babel-register --recursive tests/src",
"test-compiled": "npm run prepublish && BABEL_ENV=lib mocha --compilers js:babel-register --recursive tests/src",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i very much like avoiding use of the terrible and horrific NODE_PATH, and using babel for this instead

tests/src/package.js Outdated Show resolved Hide resolved
@adjerbetian adjerbetian requested a review from ljharb March 21, 2020 21:40
@ljharb ljharb merged commit efb5f07 into import-js:master Mar 22, 2020
@adjerbetian adjerbetian deleted the update-test-infrastructure branch March 22, 2020 09:38
@adjerbetian
Copy link
Author

@ljharb My first open source contribution! 🎉🎉🎉 As small as it is, I still celebrate 🍾

PS: if you have remarks on how I worked, please to. For instance, I'm not sure whether I should have pingued you when answering your comments.

@ljharb
Copy link
Member

ljharb commented Mar 22, 2020

Everything was fine :-) some people may only respond when they get a direct ping, perhaps if they've unsubscribed, so it's probably best to save explicit mentions for when it seems like the person will never otherwise see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants