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

Test with CircleCI 2.0 #2310

Merged
merged 4 commits into from
Jan 31, 2018
Merged

Test with CircleCI 2.0 #2310

merged 4 commits into from
Jan 31, 2018

Conversation

etpinard
Copy link
Contributor

CircleCI has mostly stopped adding new images compatible with its 1.0 framework, so it's about time we pay back some tech-debt ⛏️ 💰 In addition, upgrading to 2.0, makes our tests run in about 10 minutes - roughly 5 minutes less than for the current master. 🎉

Note that our jasmine tests now run on Chrome 59 (no more Chrome 54 as on the current master). We're using node 6.x + npm 3.x images (same as the current master). An upcoming PR (cc #1798) will bump those to node 8.x + npm 5.x - which should speed up our tests even more 🏎️

The only tricky part here was to make the image tests work. CircleCI 2.0 is setup to fetch a single docker container and run tests directly in that container. In our CircleCI 1.0 config, we fetched our imagetest docker container and made requests from a "main" container to a port mapped to the nw.js server inside imagetest. Circle 2.0 doesn't allow communication between containers, so I had to update some of our tasks/ script, mostly by removing a few hacky lines 😛

About our increasingly-annoying intermittent test failures, I still noticed some unfortunately. But hopefully this is a step in the right direction.

cc @alexcjohnson @dfcreative @scjody (who's been asking me to do this for a while)

- split into four jobs. One build job common
  to all other test jobs. One job for jasmine,
  one for image and one syntax test.
- all gl2d mocks are now tested on CI as of the regl
  `scattergl` push.
... but with --prod flag (so that it doesn't fail on
    dev-dep peer dependencies that have no effect on the dist bundles)
... to pass on CI Chrome version
@@ -10,7 +10,4 @@ npm run test-jasmine -- --tags=noCI --nowatch || EXIT_STATE=$?
# mapbox image tests take too much resources on CI
npm run test-image -- mapbox_* --queue || EXIT_STATE=$?

# run gl2d image test again (some mocks are skipped on CI)
npm run test-image-gl2d || EXIT_STATE=$?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixup for 9f304e6

No need for this line anymore as now all gl2d mocks are tested on CI.

@@ -44,7 +44,7 @@
"start-image_viewer": "node devtools/image_viewer/server.js",
"start": "npm run start-test_dashboard",
"baseline": "node tasks/baseline.js",
"preversion": "npm-link-check && npm dedupe",
"preversion": "npm-link-check && npm dedupe && npm ls --prod",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @alexcjohnson @dfcreative

This commit partly reverts 1d3a3f0

npm ls --prod will fail only if a bundle dependency (i.e. ignoring dev-dependencies) is (1) missing, (2) needs to be updated or (3) is extraneous. This step ensures that we're not bundling dist/ bundles with incorrect dependencies.

@@ -1,21 +1,29 @@
#!/bin/bash

# override CircleCi's default run settings
set +e
set +o pipefail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like CircleCI's new defaults where a given job stop executing after the first test failure. I prefer running all the tests no matter what, to see if a given commit makes multiple tests fail. I hope you agree with me.

@alexcjohnson
Copy link
Collaborator

LGTM! Only thing that occurs to me is we could balance the containers a bit more, perhaps taking the bundle tests out of jasmine and lumping with syntax, that would save another minute or two. But not a big deal, this is already a big win! 💃

@etpinard
Copy link
Contributor Author

Great. Merging. Next stop updating to node 8.x. I'll should have time to do that by week's end.

@etpinard etpinard merged commit 18dac1f into master Jan 31, 2018
@etpinard etpinard deleted the circle2 branch January 31, 2018 15:02
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.

2 participants