Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Simple Karma config for browser testing #52

Merged
merged 6 commits into from
Dec 3, 2015

Conversation

ellbee
Copy link
Member

@ellbee ellbee commented Nov 27, 2015

This is a work in progress to see if this is the direction we want to take for browser testing. You can specify the browsers you want to test with. By default it just runs the tests in Firefox.


// To specify browsers to test against use BROWSER var and comma separated list:
// BROWSER=Firefox,Chrome,Safari,IE npm run test:browser
var browsers = process.env.BROWSER ? process.env.BROWSER.split(',') : [ 'Firefox' ];
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can already specify browsers through the cli, e.g.

--browsers Chrome,Firefox

This overrides the default in the config file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, haven't tested it with npm scripts though. Does npm run test:browser --browsers Chrome,Firefox do the right thing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Played around with it, using npm run test:browser -- --browsers Chrome,Firefox works

Copy link
Member Author

Choose a reason for hiding this comment

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

No, you can't do npm run test:browser --browsers Chrome,Firefox.

However, it seems that since npm 2 you can do npm run test:browser -- --browsers Chrome,Firefox which I had no idea had been added until 5 mins ago. Great!

@kimjoar
Copy link
Collaborator

kimjoar commented Nov 27, 2015

Great stuff, @ellbee. I'll try to test this with #38 and #50.

@ellbee
Copy link
Member Author

ellbee commented Nov 27, 2015

Thanks for the review @kjbekkelund!

Usage has changed to:

npm run test:browser to run tests in firefox
npm run test:browser -- --browsers Chrome,Firefox to specify browsers

@jlongster The transform-object-assign plugin is now specified in the .babelrc and removed from the command line args of npm run build. @kjbekkelund suggested that this maybe should be a separate pull request, let me know if you want me to separate it out.

@ellbee ellbee force-pushed the simple_karma_config branch 2 times, most recently from b5e7b77 to bcbd6da Compare November 27, 2015 22:24
@kimjoar
Copy link
Collaborator

kimjoar commented Nov 28, 2015

If I change createMemoryHistory to createHashHistory 5 tests fail. I'm on a flight for 10 hours tomorrow, so I'll try to take a look at it. Would be awesome to have tests with createHashHistory work as expected.

@kimjoar
Copy link
Collaborator

kimjoar commented Nov 28, 2015

Ah, it was actually quite easy to get the tests running:

-const { createMemoryHistory: createHistory } = require('history');
+const { createHashHistory, createMemoryHistory } = require('history');
+
+let isBrowser = false;
+let createHistory = createMemoryHistory;
+if (typeof window !== 'undefined') {
+  isBrowser = true;
+  createHistory = createHashHistory;
+}
+
+beforeEach(() => {
+  if (isBrowser) {
+    window.location = '#/';
+  }
+});

It looks a little hacky, so we might want to clean it up, but at least everything is green. Maybe we should at it to this PR? What do you think, @jlongster? I can of course create a PR when this is merged if you want that.

@ellbee
Copy link
Member Author

ellbee commented Nov 28, 2015

@kjbekkelund Nice, thanks for figuring out the failures. Turns out browser history was failing too. All tests are passing now.

I have set it up so that test:node runs the tests with memory history and test:browser runs the tests with both browser history and hash history. If there are any tests we only want to run in node or the browser then they can be put in 'test/node' or 'test/browser' and will be picked up automatically.

@ellbee ellbee force-pushed the simple_karma_config branch 2 times, most recently from fc80b64 to 6c02fd7 Compare November 30, 2015 22:15
@ellbee
Copy link
Member Author

ellbee commented Nov 30, 2015

Rebased onto master.

@jlongster
Copy link
Member

Sorry for the delay. Work has been especially crazy. (if it's any consolation, I just landed a big firefox debugger patch and it officially uses redux quite extensively now!)

I hadn't looked at this and I didn't realize you moved all the tests, and I just merged another PR that updated the actions to the FSA-compliant :( It's not that hard to change, all the tests just need to check action.payload instead of the action itself. Do you mind rebasing this PR or do you want me to do it?

@ellbee
Copy link
Member Author

ellbee commented Dec 2, 2015

Hey, no worries! I don't mind rebasing, I'll get it done right now.

Not that I needed a consolation, but Firefox debugger using redux is really awesome! 👍

@jlongster
Copy link
Member

Wow, thanks! Looks great to me, I've never used karma before so good to learn. I'll merge this now if you think it's ready?

@ellbee
Copy link
Member Author

ellbee commented Dec 2, 2015

Yeah, I think it is ready. Although I have used it in the past, I am by no means a Karma expert myself. I stole pretty liberally from the test setup in React Router and History so I am confident that this is along the right lines.

@jlongster
Copy link
Member

Awesome, I'll merge and then try it out.

There's another PR suggesting setting up TravisCI, I wonder if I can run browser stuff there.

jlongster added a commit that referenced this pull request Dec 3, 2015
Simple Karma config for browser testing
@jlongster jlongster merged commit 8422f9f into reactjs:master Dec 3, 2015
@ellbee ellbee deleted the simple_karma_config branch December 3, 2015 07:00
@ellbee
Copy link
Member Author

ellbee commented Dec 3, 2015

I think Travis has Firefox, so should be fine.

Edit: https://docs.travis-ci.com/user/gui-and-headless-browsers/#Karma-and-Firefox-inactivity-timeouts

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

Successfully merging this pull request may close these issues.

3 participants