679490 - Jetpack tinderbox reports success for test runs that fail tests

Status: RESOLVED FIXED

Description

[Myk Melez [:myk] [@mykmelez]]

The Jetpack tinderbox reports success (i.e. green) for test runs that fail tests.  Here's an example:

http://tbpl.mozilla.org/?tree=Jetpack&rev=22a8e6fa5ff1

From what I can tell, right now it unconditionally reports success, no matter what actually happens during each run. It should be reporting failure (i.e. red) for test runs with any failing tests.

Comment 1

[Phil Ringnalda (:philor)]

Isn't this bug 669520? I don't get why lsblakk resolved that, since it didn't seem to need a same-rev example so much as needing the code running the tests on the Jetpack tree to do the same scraping for "Traceback" that the code running the tests on mozilla-central does.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Phil Ringnalda (:philor) from comment #1)
> Isn't this bug 669520? I don't get why lsblakk resolved that, since it
> didn't seem to need a same-rev example so much as needing the code running
> the tests on the Jetpack tree to do the same scraping for "Traceback" that
> the code running the tests on mozilla-central does.

You're right, I'm not sure where I got confused on that last bug. So back to setting up error parsing on the ScriptFactory that matches what is run on m-c test suite runs.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Does http://pastebin.mozilla.org/1324293 look like it gets what you need? http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest is super backlogged so in about an hour the results for that push should show up there too and I can paste the full log.  This is just a run that I purposely messed with on my local machine so that it would have at least one test failure.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Created attachment 558862 [details] [diff] [review]
v.1 run_jetpack with error parsing for addonsdk-poller runs

screenshot of tinderbox when a test fails (local machine testing): http://cl.ly/3A2t2c3k122w311I1z1k

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

filed bug 679490 since we'll need to move this away from Tinderbox eventually.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to Lukas Blakk [:lsblakk] from comment #3)
> Does http://pastebin.mozilla.org/1324293 look like it gets what you need?

It looks right to me, but Brian is the expert.

Brian: does that output look right to you?


(In reply to Lukas Blakk [:lsblakk] from comment #4)
> screenshot of tinderbox when a test fails (local machine testing):
> http://cl.ly/3A2t2c3k122w311I1z1k

Here's a live URL of that view:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTest&maxdate=1315395117&hours=24&legend=0&norules=1


(In reply to Lukas Blakk [:lsblakk] from comment #5)
> filed bug 679490 since we'll need to move this away from Tinderbox
> eventually.

Did you mean to cite a different bug number here?  In any case, yes, the tbpl view is what we really care about:

https://tbpl.mozilla.org/?tree=Jetpack

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

> Did you mean to cite a different bug number here?  In any case, yes, the
> tbpl view is what we really care about:
> 
> https://tbpl.mozilla.org/?tree=Jetpack

Why yes I did: bug 685299 

The tbpl you link there *should* work, although slowly and with suffering due to the tinderbox mail backlog that we are in right now. I believe most of the tbpl pages are using usebuildbot=1 now for more swift reporting and that's where the bug I filed comes into play - we need to be able to link those logs in the build database with an ftp storage of the logs in order to move away from reliance on tinderbox for results & error parsing.

Comment 8

[Brian Warner [:warner :bwarner]]

(In reply to Myk Melez [:myk] from comment #6)
> (In reply to Lukas Blakk [:lsblakk] from comment #3)
> > Does http://pastebin.mozilla.org/1324293 look like it gets what you need?
> 
> It looks right to me, but Brian is the expert.

That certainly looks like the output of a jetpack test process, and if I were writing some buildbot code to parse it, it's what I'd need to write that code. Did I answer the right question? :)

But, can't we just have the buildstep look at the exit status of the test process and declare failure if-and-only-if it's non-zero? That'd be a lot more reliable and wouldn't misfire when e.g. we have a test named test-traceback.js or test-failure.js . Scanning for those messages to produce a condensed logfile sounds reasonable, but scanning them to determine success-vs-failure feels fragile to me.

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 558862 [details] [diff] [review]
v.1 run_jetpack with error parsing for addonsdk-poller runs

Review of attachment 558862 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. As discussed, let's grab the exit code from the child process and return that.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Created attachment 559346 [details] [diff] [review]
v.2 run_jetpack with error parsing for addonsdk-poller runs, now exits with subprocess.Popen returncode

tested on my local machine with a snowleopard jetpack-poller builder and a 10.6 mozilla-central debug jetpack builder to make sure nothing broke for branch runs while adding this to addonsdk-poller triggered runs.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 559346 [details] [diff] [review]
v.2 run_jetpack with error parsing for addonsdk-poller runs, now exits with subprocess.Popen returncode

landed: http://hg.mozilla.org/build/tools/rev/8015140cb437

This should go into effect immediately as the run_jetpack script is downloaded per run. Please let me know if any undesired behaviour occurs.

Comment 12

[Armen [:armenzg] (back on March 4th)]

It actually happened but the switch from tinderbox to buildbot based tbpl bit us.

The jetpack jobs were not showing anymore. See bug 687021 to see what happened for win64. 

This has been failing since then with ImportError: No module named sut_lib [1]

I have back the change out:
http://hg.mozilla.org/build/tools/rev/0c0665c0fbb9
The backout has fixed the problem.

[1] Log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1316184472.1316184557.9239.gz&fulltext=1

Comment 13

[Phil Ringnalda (:philor)]

(In reply to Armen Zambrano G. [:armenzg] - Release Engineer from comment #12)
> It actually happened but the switch from tinderbox to buildbot based tbpl
> bit us.

I'm pretty sure that time's arrow points the other way. If it landed and took immediate effect on the afternoon of the 9th, and we switched to usebuildbot by default on the late evening of the 12th, then there were 3.5 days before "buildbot based tbpl bit us" during which "someone hid it on tinderbox based tbpl, and nobody cared that it was gone, bit us."

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Created attachment 561499 [details] [diff] [review]
v.3 run_jetpack with error parsing for addonsdk-poller runs, now without importing sut_lib

Test runs in branch test suites use a different naming scheme/location for the tools dir - in the addonsdk poller triggered test runs it's called 'scripts'. Instead of casing the chasing around of sut_lib I have just added the little runCommand that is needed to the run_jetpack.py script and ran tests in both m-c jetpack test as well as jetpack-snowleopard on my local setup.  Both have passed (the reason this wasn't caught in my previous testing was that my m-c test run was actually pulling the build/tools and not my user repo tools - separate path settings).

Comment 15

[Armen [:armenzg] (back on March 4th)]

Comment on attachment 561499 [details] [diff] [review]
v.3 run_jetpack with error parsing for addonsdk-poller runs, now without importing sut_lib

Review of attachment 561499 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the addition of the note.
The interdiff was pretty clear between the two patches.

::: buildfarm/utils/run_jetpack.py
@@ +13,5 @@
>  SDK_DIR="jetpack"
>  
> +log = logging.getLogger()
> +
> +def runCommand(cmd, env=None, logEcho=True):

Can you please add a note to point to the code you are forking off this code?

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 561499 [details] [diff] [review]
v.3 run_jetpack with error parsing for addonsdk-poller runs, now without importing sut_lib

I added a note in both run_jetpack.py and sut_lib.py referencing the copying of runCommand.

Landed: http://hg.mozilla.org/build/tools/rev/edca76df663e

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

This is live and seems to be working as desired now.