-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Update test infrastructure and babel #1686
Conversation
There was a problem hiding this 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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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 |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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 😅.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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
@ljharb I simplified my modification to only make my local tests pass. What do you think? |
There was a problem hiding this 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!
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", |
There was a problem hiding this comment.
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
@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. |
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. |
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
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 thelodash.isarray
dependency that was assumed to be present in some tests, so I added it manually.