1225559 - Gij Test Suite skipping some tests

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect, P1)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

For instance, in this logfile [1], we see that the test "will run apps/homescreen/test/marionette/pinning_the_web_test.js". However we never see TEST-START or TEST-END status for that test even though the chunk returns as passing. I know for a fact that this test should be perma-failing because I broke it with an earlier commit. I can confirm this test-missing is happening with at least 4 tests:

apps/homescreen/test/marionette/pinning_the_web_test.js
apps/homescreen/test/marionette/app_default_order_test.js
apps/homescreen/test/marionette/app_order_test.js
apps/homescreen/test/marionette/app_reorder_hint_test.js


1.) https://public-artifacts.taskcluster.net/QlVTWqw-TAumda2Yc5fh4g/0/public/logs/live_backing.log

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

Gareth, Aus, any ideas here?

Comment 2

[David Flanagan [:djf]]

Could the issue be that the JS in the test files is so broken that the test() functions never get called? Given that one .js file can have multiple tests within it, it looks like Marionette can't know about the existance of a test unless the code correctly registers a test. 

Maybe the test runner should flag an error if it tries to run a test file and that file exits without ever actually registering any tests to run.

Comment 3

[David Flanagan [:djf]]

In the log that Michael provides above, I see that there is a retry on a previous test that failed. Could there be something wrong with the retry logic where retrying a test causes future tests to be skipped? That seems unlikely, but could be checked by running with the patch that disables retries, I suppose.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

The thing is, I'm seeing skipped tests in a lot of test chunks. If I click on one at random on gaia-master [1], it seems there are skipped tests. Also, here is a chunk that does not have any retries in it, but is skipping most tests [2].

1.) https://treeherder.mozilla.org/#/jobs?repo=gaia-master
2.) https://public-artifacts.taskcluster.net/dKyKb5U6QGeuE0qpdS_KCA/0/public/logs/live_backing.log

Comment 5

[Aus Lacroix [:aus]]

(In reply to Michael Henretty [:mhenretty] from comment #0)
> For instance, in this logfile [1], we see that the test "will run
> apps/homescreen/test/marionette/pinning_the_web_test.js". However we never
> see TEST-START or TEST-END status for that test even though the chunk
> returns as passing. I know for a fact that this test should be perma-failing
> because I broke it with an earlier commit. I can confirm this test-missing
> is happening with at least 4 tests:
> 
> apps/homescreen/test/marionette/pinning_the_web_test.js
> apps/homescreen/test/marionette/app_default_order_test.js
> apps/homescreen/test/marionette/app_order_test.js
> apps/homescreen/test/marionette/app_reorder_hint_test.js
> 
> 
> 1.)
> https://public-artifacts.taskcluster.net/QlVTWqw-TAumda2Yc5fh4g/0/public/
> logs/live_backing.log

Dude, yep. I noticed your perma-fail commit because I fixed it in my PR for bug 1222215. This would indicate a problem with the retry logic. I'm assigning this to myself, hope to have the rest figured out soon.

Comment 6

[Aus Lacroix [:aus]]

Well, good news and bad news.

Good news is, this issue *SHOULD* be compartmentalized. It only occurs when a test file (e.g. apps/clock/test/marionette/timer_test.js) fails to start running but there are *OTHER* files in the TEST_FILES that will be tested that run and pass.

We have logic in place to detect catastrophic failure where we expected to run many test files but didn't run any individual tests. It does NOT, however, cover this particular case.

The only safe thing for us to do to ensure we catch these is to complete _HALT_ when we get this type of failure.

I'm working on a patch for this right now.

With retries disabled, this is a non-issue and the failure is clear and present.

Comment 7

[Aus Lacroix [:aus]]

Attachment: Pull Request - Treat marionette-mocha errors as fatal.

Comment 8

[Aus Lacroix [:aus]]

OK, this is the right idea, but when we run tests, marionette is really chatty and will output things with 'Error:'. I'm trying to batten down the error message we should look for when we have the type of fatal error that occurs when the file fails to parse, or fails to even execute one test() function.

Comment 9

[Aus Lacroix [:aus]]

Latest test run looks good. I think we're pretty set on this one. I'm going to go ahead and land this and make sure master is stable, then we can go ahead and reopen the tree.

:mhenretty will have a chance to give post check-in feedback and we can adjust if needed but tree availability is of highest concern.

Comment 10

[Aus Lacroix [:aus]]

test run --> https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=e4bdf230cd6659bb7d9d0eb3c965a9f2d08a8d9b

Comment 11

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/cba7e4b86361af31b153cfebaf99900e0b860f7b

I'm going to wait on marking this fixed until the review is completed.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8688748 [details] [review]
Pull Request - Treat marionette-mocha errors as fatal.

I'm giving a posthumous r+, since it looks to improve the situation.

However, we are still seeing the original problem even after this commit. For instance, in the lastest test run on master, if I click the first chunk [1], I see that `apps/calendar/test/marionette/alarm_test.js` is supposed to be run, but we never see output from that test. Let's leave this bug open for further investigation.

1.) https://public-artifacts.taskcluster.net/e2KBD9tFQASwSJH83YnwyA/0/public/logs/live_backing.log

Comment 13

[Aus Lacroix [:aus]]

(In reply to Michael Henretty [:mhenretty] from comment #12)
> Comment on attachment 8688748 [details] [review]
> Pull Request - Treat marionette-mocha errors as fatal.
> 
> I'm giving a posthumous r+, since it looks to improve the situation.
> 
> However, we are still seeing the original problem even after this commit.
> For instance, in the lastest test run on master, if I click the first chunk
> [1], I see that `apps/calendar/test/marionette/alarm_test.js` is supposed to
> be run, but we never see output from that test. Let's leave this bug open
> for further investigation.
> 
> 1.)
> https://public-artifacts.taskcluster.net/e2KBD9tFQASwSJH83YnwyA/0/public/
> logs/live_backing.log

Totally. I fixed the instance where the retry wouldn't catch that the test file itself wouldn't start running because the top level marionette() entry point would throw but it seems to be, for some very odd reason, skipping over certain tests anyway.

Comment 14

[Aus Lacroix [:aus]]

Hmmm, that file is present in the tbpl-manifest.json file that prunes files from the test list.

There are two ways for a test to get excluded. It can have 'skip()' at the top of it. Which will say the test is 'PENDING' or 'TODO' I believe. Then there's the tbpl-manifest.json file in shared/test/integration (https://github.com/nullaus/gaia/blob/master/shared/test/integration/tbpl-manifest.json).

I think it gets confusing because we show the original, non pruned list. 

I'll change it so we output the list of test files that will _actually_ have output, even if those are skipped later via skip().

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] nullaus:bug1225559 > mozilla-b2g:master

Comment 16

[Aus Lacroix [:aus]]

Commit (master): https://github.com/mozilla-b2g/gaia/commit/a73d6ac804d3b5041f6fa8061dba4f62df6cc039

Second round. This fixes the issue where we were erroneously reporting which test files would get run _before_ applying the disabled tests manifest file (aka tbpl-manifest.json) which made it appear like some test files that shouldn't have been skipped were completely omitted.

I've verified this on the latest test run and everything looked correct. I'm going to go ahead and mark this fixed.

Mike, would still like you to have a look at it, we can always fine tune how we report the list of files. Right now they're each on their own line, sorted alphabetically.

Comment 17

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8689175 [details] [review]
[gaia] nullaus:bug1225559 > mozilla-b2g:master

Great work Aus! Everything is looking good :)