tests: use stages for the travis execution

[come-maiz]

Fixes #694

🚀 Description

Use travis stages for better control of the things to run in parallel.

• 📘 I've reviewed the OpenZeppelin Contributor Guidelines
• ✅ I've added tests where applicable to test my new functionality.
• 📖 I've made sure that my contracts are well-documented.
• 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

[come-maiz]

I'm not super sure about fast finish and allow failures. But it's late and travis is grumpy and not taking this PR. So, WIP, please don't merge it yet.

[come-maiz]

Okay, ready for review.

[nventuro]

I'd add a short comment explaining we're running 3 jobs in the unit stage, since Travis' syntax for this is so obscure.

[come-maiz]

Thanks for the review @nventuro!
I've added comments, please take another look.

[nventuro]

Apparently Travis supports explicitly indicating the stage for each job: I think this would make the intent clearer.

This config:

  include:
    - stage: static
      script: npm run lint
    - stage: unit
      script: npm run test
    - stage: unit
      script: npm run test
      env: SOLIDITY_COVERAGE=true
    - stage: unit
      script: npm run test
      env: SOLC_NIGHTLY=true

produces the same result.

[come-maiz]

@nventuro updated.

[frangio]

I really like this but I don't like giving up fast_finish. I think it's important that builds be as fast as possible, and coverage analysis is super super slow.

I'd put this on hold for now.

[frangio]

Yay! This was in my mind too.

[frangio]

If we put this PR on hold we should still do this now.

[frangio]

I don't think "Static" is a very clear name. Have you considered "Linting"?

[come-maiz]

wfm.

[frangio]

How about "Unit tests" for the stage name?

[come-maiz]

sounds good.

[frangio]

@ElOpio Can you explain a bit further the motivation behind this? Were you looking to put linting in a separate job? If so, what is the problem with it running in parallel with everything else?

[come-maiz]

@frangio yes, the initial requirement was to not run linting on the coverage job, as requested on #694.
Doing that with the test matrix on travis is very ugly, because you have to pass flags in env vars, so I used stages instead.

In testing, to make the best use of human and machine resources, you build a pyramid. The base of the pyramid is clean code, then testing functionality of units, and then testing integrations and the system viewed from a higher level.
So, following that, it made sense to run static as the first step in the sequence. Now we inverted the pyramid so we won't save any human resources, and the machine resources are not ours. I'm ok now running everything in parallel.

About failfast, I prefer to sacrifice those 10 minutes before the squash button is enabled instead of using the matrix. Knowing how travis works, this is not likely to be fixed fast. But if you prefer to keep failfast, I can modify the test script to make the matrix not so ugly.

[frangio]

In testing, to make the best use of human and machine resources, you build a pyramid.

This is nice, I had never heard of it.

the machine resources are not ours

For the record, Travis does have limits on parallel use of resources, but I don't think it would affect us too negatively in this case.

I prefer to sacrifice those 10 minutes before the squash button is enabled instead of using the matrix

I'd love to get a third opinion on this before moving forward with either option. @nventuro?

[nventuro]

Fast finish does work, but stages won't advance until all jobs (even those that are allowed to fail) finish. So we could have the best of both worlds, by putting the linter stage first. Given our decision of not treating aesthetic linter errors as build errors, this is fine by me.

[frangio]

It's still not clear to me if having the unit tests as the last stage will result in fast_finish actually causing the entire build to fail once the tests fail in the non-coverage non-nightly job.

[nventuro]

I'll do some tests on this and come back with more information.

[frangio]

Hey stages are now stable, since last month. 🙂

https://blog.travis-ci.com/2018-07-18-build-stages-officially-released

[frangio]

It seems like linting will still not run until all jobs including coverage have finished.

[come-maiz]

Wow, and the diff is now looking super ugly.

@frangio we could run linting in parallel with any of the stages, but now I'm not sure what you are looking for :D
@nventuro convinced you make the linter fail on error, so why not move it back as the first step of the test run?

[nventuro]

@ElOpio we'd like for:

a) linting errors to cause a build error, but still allow for the test suite to run (i.e. don't run linting first and abort the build on error)
b) linting errors to not wait until the coverage test has finished, since that can take very long

[come-maiz]

@nventuro So, all of them running in parallel. That is, no stages.
But, I still prefer the stages syntax over the matrix, and #1186 will look a lot better as a conditional stage.
I will make a simple change to run all of them in parallel.

[come-maiz]

@nventuro @frangio please take another look. But we could argue here endlessly about the details, I would prefer to land it and do small tweaks when we have a reason to complain about the way the tests are executed.

[frangio]

@ElOpio Yay. You're completely right that stages syntax is much much better than the matrix. I'm almost ready to merge this now. Just one small tweak: can we add a name: field to each job? (See here.)

[frangio]

Thanks @ElOpio!