Make local tests the same as travis tests



Firefox OS
4 years ago
4 years ago


(Reporter: daleharvey, Assigned: daleharvey)


1.4 S2 (28feb)
Mac OS X

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [systemsfe][p=8])


(1 attachment)

The commands we use to run test locally should be the same code path travis uses to run the tests
Assignee: nobody → dale
Whiteboard: [systemsfe]
Unassigning since I am not currently working on it, but adding it to my tracking 'when I have time / it annoys me too much' things

Proposal, travis runs |make test| to run all the tests, we run |make test| to run all the tests, if the failures arent the same, its a bug. |make test| looks something like: 

    test: hint unit marionette gaia-ui build-test

Each of those commands we can run individually, if they dont have the same failures as travis, its a bug, the things where we dont want to go through the full travis routine are optimisations on this process (where we dont want to download a fresh gecko every time etc)

This gets rid of the before / after scripts and consolidates the ./bin/gaia-X scripts into the same workflow that travis uses so we arent doing silly things like testing with vastly different profiles etc
Assignee: dale → nobody
Blocks: 965201
If travis is simply running "make test" then we lose the nice parallel view. I agree we could still run a parallel make build, but then the output would be garbled.

That said, I agree that we move towards a model where it's very easy as a developer to run the same tests than Travis.

IMO this should be done by:
* using a set of individual shell scripts for install/before-script/script, maybe reuse directly the travis ones
* make the scripts in bin/ use them directly
* add scripts in bin/ for each of Travis jobs

I'm not sure using the Makefile as simple launcher is such a good idea, but I don't really mind.
ah yeh good point, I wasnt meaning to change how parallel execution works, the travis build matrix can just be 

 - make jshint 
 - make test-integration 

It could, but I'd like to dig deeper why we have before_script and install. Eg is it useful to have them as separate scripts or not?

I think this at least enables folding in the Travis output but I don't know if there are other effects.
Assignee: nobody → dale
Lets see what happens,
Created attachment 8367991 [details] [review]

Removes the before_script and install steps from travis and exposes new make test-X commands which travis uses (currently undocumented). The after_script made sense to leave.

Individually the make test-X arent finished, make lint / make test-lint should be merged with the install of closure done after checking if its installed for example. The aim should be to get rid of ./tests/travis_ci/x/script

I think we should merge this then file bugs for the individual targets, attempting to do it all at once will just leave this to rot
Attachment #8367991 - Flags: review?(felash)
Comment on attachment 8367991 [details] [review]

Left comments on github.

I think this is desirable to have the setup scripts run separately in travis so we need another slightly more complex way to do it properly, IMO.
Attachment #8367991 - Flags: review?(felash)
Comment on attachment 8367991 [details] [review]

Agreed with the changes to marionette-js / marionette-python, I have done those. 

As for the output, as we discussed on IRC, I have followed the commit up with a change to remove make test-lint, I plan to do that for all the targets, test-integration and build_steps should be similiarly small

While it seems to be the same amount of complexity to have a make target that we run that runs the travis scripts sequentially, it means they are left in the current situation where we have a 'default' make target (like make lint or make test-integration) that doesnt perform the full steps to run the test, is makes it confusing at where the entry point is and it means understanding how a command flows between 15 files instead of just checking 'what does the lint target do'

I dont think our travis output is in a good state, we can improve it, but I dont think having more text to scroll in travis is a good reason to keep an extra 15 files that both complicate the test flow and break the assumption that what devs run = what travis runs, so rerequesting a review on this, hopefully its clearer what I mean / why its the right thing to do.
Attachment #8367991 - Flags: review?(felash)
Blocks: 965185
No longer blocks: 965201
Duplicate of this bug: 969233
I launched a PR with some triggered issues because I want to see how errors look like when it's not collapsed. =>
Ok, seems like uncollapsed scripts still look ok in Travis, let's move forward.

If we gonna use the Makefile, let's use its dependency capability as well.
I dont want to go 100% makefile, I just want the local tests to be the same as the travis tests and yes I want us to stop doing the same thing in multiple places, I moved the b2g target out of the integration test which is landing @

Can we review / land this as it is? its obvious improvement, there are lots more that can be made but that depends on code landing.
Comment on attachment 8367991 [details] [review]

r=me with the nits

as discussed on IRC, we should wait for bug 934952 to land soon and convert the new job to this style as well.

Can you also file a bug to better use make's capabilities?
Attachment #8367991 - Flags: review?(felash) → review+
Target Milestone: --- → 1.4 S2 (28feb)
Whiteboard: [systemsfe] → [systemsfe][p=8]
So I am gonna close this, I dont think its a good idea to land a big 'redo the way all the tests run' right in a crucial stabilisation period, however this is the cause of a fair amount of bugs that I have been seeing so I will fix them incrementally as I hit them.

I would like it kept in mind for future reviews and work on the tests, its fairly uncontroversial, the closer to how we run the tests to how travis and tbpl run the tests, the less things break, things like how the code folds in travis is utterly inconsequential compared to how easily and reliably developers can reproduce test failures.
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.