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

Tests: Clarify the travis tests using build stages #3150

Merged
merged 4 commits into from
Oct 26, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 25, 2017

This PR rewrites the .travis.yml config file to make use of the new Build Stages feature. This feature has the following pros:

  • Dropping the matrix allows us to drop the useless env variables we're using to know which script to run
  • Each job has its own entry with its own config, makes it easy to add new jobs with custom configs (I'll add one soon for e2e tests)
  • Organize the jobs in "stages": stages run sequentially but jobs in the same stage run in parallel. (see https://travis-ci.org/WordPress/gutenberg/builds/292669637)

@youknowriad youknowriad added the [Status] In Progress Tracking issues with work in progress label Oct 25, 2017
@youknowriad youknowriad self-assigned this Oct 25, 2017
@codecov
Copy link

codecov bot commented Oct 25, 2017

Codecov Report

Merging #3150 into master will decrease coverage by 0.77%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
- Coverage   31.63%   30.85%   -0.78%     
==========================================
  Files         217      217              
  Lines        6275     6511     +236     
  Branches     1116     1196      +80     
==========================================
+ Hits         1985     2009      +24     
- Misses       3606     3744     +138     
- Partials      684      758      +74
Impacted Files Coverage Δ
blocks/library/code/index.js 50% <0%> (-7.15%) ⬇️
blocks/editable/format-toolbar/index.js 5.19% <0%> (-1.19%) ⬇️
editor/block-toolbar/index.js 0% <0%> (ø) ⬆️
editor/writing-flow/index.js 0% <0%> (ø) ⬆️
blocks/editable/index.js 11.96% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d463eb0...e527081. Read the comment docs.

@youknowriad youknowriad added [Type] Build Tooling Issues or PRs related to build tooling and removed [Status] In Progress Tracking issues with work in progress labels Oct 25, 2017
@youknowriad youknowriad requested review from gziolo and aduth October 25, 2017 15:21
.travis.yml Outdated
stages:
- lint
- unit
- test
Copy link
Member

Choose a reason for hiding this comment

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

There's no test stage unless I'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I had a test stage (it's the default stage for the default script) but removed it in the last commit

@aduth
Copy link
Member

aduth commented Oct 25, 2017

From what I understand of this feature, I'm not sure we would want to define linting and unit tests as separate stages, since there's no need for them to occur sequentially?

https://docs.travis-ci.com/user/build-stages/

@youknowriad
Copy link
Contributor Author

since there's no need for them to occur sequentially??

That's a choice you'd have to make on your own. The idea was to fail early in the process if it's a linting issue, make things clearer IMO but I'm open to using one stage for now.

package.json Outdated
@@ -126,7 +126,7 @@
"lint": "eslint .",
"dev": "cross-env BABEL_ENV=default webpack --watch",
"test": "npm run lint && npm run test-unit",
"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
"ci": "concurrently \"npm run build\" \"npm run test-unit:coverage-ci\"",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to execute npm run build here, too? It's executed as part of bin/run-wp-unit-tests.sh for every PHP version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added later to the PHP unit tests to build the parser. If the parser is no longer built this way in the future (say a Pure PHP parser), this could be removed.

While keeping it here ensure the build is tested as part of the JS unit tests which is more logical to me.

Copy link
Member

Choose a reason for hiding this comment

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

We moved lint out, so we could do the same with the build, and call npm run test-unit:coverage-ci directly from Travis. We can leave it as it is, too. I don't think it makes any difference in the end.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

From what I understand of this feature, I'm not sure we would want to define linting and unit tests as separate stages, since there's no need for them to occur sequentially?

Is it going to be faster to run everything in parallel assuming that all jobs pass? If yes, I would be also in favor of having everything in parallel.

I have no experience with more advanced setup of travis.yml, but this looks good. It seems like adding another job as a very straightforward task.

@youknowriad
Copy link
Contributor Author

youknowriad commented Oct 26, 2017

Is it going to be faster to run everything in parallel assuming that all jobs will pass?

Yes, but if linting fails, we'd have lost time. But I understand the point here. Linting don't break often. I just like the logical flow: linting => unit tests => integration tests => deploy

Also, there's a limit in concurrent jobs in Travis so the gains are smaller. Though, I'm not sure what's our current limit.

Anyway, I'm updating to run everything in parallel

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

This looks solid, all jobs pass. I compared build times with what we had before and I didn't see noticeable changes. You might want to wait for a second review from someone more familiar with Travis :)

@youknowriad
Copy link
Contributor Author

Merging, I need to to add job in #3069

@youknowriad youknowriad merged commit 1b444cb into master Oct 26, 2017
@youknowriad youknowriad deleted the try/travis-stages branch October 26, 2017 12:47
@aduth
Copy link
Member

aduth commented Oct 26, 2017

I like the parallelization. I personally don't see the need for a sequence where linting occurs before unit tests. To your point on wasted time for a failure, I think we ought to optimize for timing of a success case, of which parallelizing should improve (in theory anyways).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants