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

Updating the version of node on travis so that ES6 is supported #816

Merged
merged 15 commits into from
Feb 20, 2018

Conversation

lobax
Copy link
Contributor

@lobax lobax commented Feb 15, 2018

Attempting to change the version of node used on travis as per the suggestion given by @wlach in issue #815

@lobax
Copy link
Contributor Author

lobax commented Feb 15, 2018

The tests are not preformed on travis. It produces the following error and then stalls until it times out:

Error: Can't set headers after they are sent.
    at validateHeader (_http_outgoing.js:494:11)
    at ServerResponse.setHeader (_http_outgoing.js:501:3)
    at ServerResponse.header (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/response.js:767:10)
    at ServerResponse.send (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/response.js:170:12)
    at /home/travis/build/DD2480-G9/metrics-graphics/node_modules/testem/lib/server/index.js:93:27
    at Layer.handle_error (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/router/layer.js:71:5)
    at trim_prefix (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/router/index.js:315:13)
    at /home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/router/index.js:284:7
    at Function.process_params (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/router/index.js:335:12)
    at next (/home/travis/build/DD2480-G9/metrics-graphics/node_modules/express/lib/router/index.js:275:10)

@wlach
Copy link
Collaborator

wlach commented Feb 15, 2018

When in doubt, try upgrading node packages. :) Could you try upgrading the version of testem to 2.0.0 and see if it helps?

https://github.com/mozilla/metrics-graphics/blob/4f4f702fcd4cebd19ee894e89e8860afa11f6e47/package.json#L59

@RickardBjorklund
Copy link

RickardBjorklund commented Feb 15, 2018

Applying the changes @wlach suggested in #815 and #816 (specifying node version in travis.yml and upgrading node package) makes travis run again 13c71a1 however when applying a trivial ES5 to ES6 change 8ae73fb all tests related to the updated file crashes and fails in travis. Applying the same change and running the test suite locally with gulp test works well and no test crashes or fails.

@lobax
Copy link
Contributor Author

lobax commented Feb 16, 2018

My guess is that the tests on the CI fail because Phantomjs 2.1 does not support ES6. Development on PJS seems rather stale and they do not support ES6 yet, so perhaps testing should be done in a different way.

@wlach
Copy link
Collaborator

wlach commented Feb 16, 2018

@lobax @RickardBjorklund argh that sucks, I didn't know phantomjs didn't support es6

In any case, we should probably move to doing headless browser testing using Firefox and Chrome. Travis has some general advice on how to do this:

https://docs.travis-ci.com/user/gui-and-headless-browsers/

I'm not 100% sure how to hook this up to testem, but I can't imagine it's that hard. For a first pass, I think we can settle for testing using either Firefox or Chrome, unless it's super trivial to do both. We can always add support for the other in a follow-up.

Thank you for your patience on this!

@lobax
Copy link
Contributor Author

lobax commented Feb 18, 2018

@wlach It now runs fine on Chrome instead of phantomjs. It probably shouldn't run on Firefox until #814 is fixed.

Please double check the rows I added to the travis.yml file though, as those changes where needed to get chrome to run on our fork but it seemed to work fine without them here.

@wlach wlach merged commit 2a49786 into metricsgraphics:master Feb 20, 2018
@wlach
Copy link
Collaborator

wlach commented Feb 20, 2018

Just squashed this into one commit and landed it. Thank you!

thomaschampagne pushed a commit to thomaschampagne/metrics-graphics that referenced this pull request Mar 7, 2018
* Use Chrome for testing
* Update version of node
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants