-
Notifications
You must be signed in to change notification settings - Fork 468
test: Run in node #377
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
test: Run in node #377
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.
Thanks! The last time we tried to get these tests to run in node it was way more complicated, but this is pretty simple so I'm happy with that. Thank you for your work on this.
'/__tests__/', | ||
'/__node_tests__/', | ||
], | ||
testMatch: ['**/__node_tests__/**.js'], |
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.
Should we move the existing file(s) to regular test files (or just remove them?)
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'll think about that when we're getting closer to green tests 😄
@@ -0,0 +1,6 @@ | |||
module.exports = { |
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.
We should remove the eslintConfig
property from the package.json and copy some of the overridden rules to this file.
Also the extends can be: require.resolve('kcd-scripts/eslint')
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.
Ah it was in the package.json. I was wondering where that was coming from.
Also the extends can be: require.resolve('kcd-scripts/eslint')
👍
Yes (provided it doesn't put any unnecessary strain on maintenance of the library). It's not too difficult to support, and there are users who rely on this, so we support it. But if it becomes too much of a chore, then we can drop support I think. I'd like to hear what other people think. A few months ago there was one contributor who was very vocal about the need for this support. I'd like to know what everyone thinks about supporting this. |
I don't have a good answer how to test some tests in multiple environments. I think it makes more sense to add smoke tests for specific methods (like in #378) and run proper tests with a global DOM. |
I have to ask again: Is the purpose of this to be importable/evaluateable in node or useable without a global DOM?
This will require some work and before deep diving I want to be sure I'm not just implementing someones "I like".
If we don't care about this requirement here why do we care for it in
jest-dom
? It lowers interop with other dom related libraries since everybody is assuming that at least window or document are globally defined.I traced the requirement back to testing-library/jest-dom#58 which is quite a different thing to ask. Yes instanceof checks need special treatment for cross-realm elements but those are fixable (though difficult to test properly).
But asking for no global DOM available was not part of the question, It was explicitly not asked by providing the quote from jsdom
This doesn't mean you shouldn't expose a global window. Just that you don't mix these environments but rather execute your scripts inside jsdom which would result in a global
window
.You can't just cherry-pick this quote and not expose a global window but still run in the node environment.
/cc @kentcdodds