-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
running tests with karma #421
Conversation
3e0b7f3
to
c42d0a0
Compare
this is ready to merge now I believe 😀 |
before_script: | ||
- "sh install-relevant-react.sh" | ||
- if [ $KARMA == 1 ]; then export CHROME_BIN=chromium-browser; fi | ||
- if [ $KARMA == 1 ]; then export DISPLAY=:99.0; fi |
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.
can all three of these commands be combined into one line, if they're related?
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.
good point, I've combined them
3ec1838
to
cc386f6
Compare
3 tests fail when ran on Firefox 45.0 and react 15.1.0 with karma because of this firefox bug facebook/react#6896 it'll be fixed in a future version of react (or firefox) so I don't think there's anything for us to do here other than be aware of it. |
1fc2967
to
ad37536
Compare
oops, accidentally closed this, will reopen tomorrow |
@nfcampos what's the status on this? |
I'll rebase and after that it should be ready to go |
7747c83
to
d9fbe3e
Compare
@aweary do you want to take a look at this before it gets out of date again? :) |
27d488e
to
4672e04
Compare
5d06c6a
to
dee5cba
Compare
I've rebased this on the current master, does someone want to review this? |
|
||
var VERSION = React.version; | ||
|
||
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.
why is this file changing from ES6 exports to node 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.
because I need to import it in the karma conf file and I don't want to transpile that
makes sense?
the other option is to require('babel-register')
at the beginning of the karma conf file. would that be better?
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'd prefer npm run build
before the karma tests, so that the tests are always running on the build output.
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.
hmm ok, makes sense. so i guess the test:karma
command in package.json should run build
before? or we should do that manually when testing with karma?
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.
hmm, actually that's pretty inconvenient, because the test files themselves import the source files from src
, not from build
, what do you suggest for that?
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.
hm, that's a good point. since the mocha tests use babel-register, let's just use that here too.
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.
done
], | ||
|
||
preprocessors: { | ||
'test/*.jsx': ['webpack', 'sourcemap'], |
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.
.js
might need preprocessing too - we use babel for every file, and .jsx
is only for files that contain JSX
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.
good point, will change
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've changed this
00621f9
to
ed1d057
Compare
does anyone else want to review this? |
4f51d06
to
d7f1b58
Compare
- rewrote src/version.js in ES5 to be able to use it in karma.conf.js - conditionally add webpack.IgnorePlugin based on React version installed, mirrorring react-compat.js
LGTM |
where this is at:
2 are failingfixed now (were failing because the tests were assuming the body was empty)only running in chrome for nowchrome and firefoxtodo