Just spent a couple of days trying to debug a test where it turns out that an earlier test threw a silent exception and didn't clean up after itself. When a test has zero passes and zero fails it is clear that something has gone wrong so we should log this as a failure.

Comment on attachment 8513461 [details] [diff] [review] zero-passes-zero-fails-should-fail-1090913.patch Review of attachment 8513461 [details] [diff] [review]: ----------------------------------------------------------------- awesome! by chance was that previous test in a different directory?

That patch failed when ran against multiple tests. Anyhow, here is an updated patch that definitely works with single tests and also when a batch of tests is run. Suppose I should ask for review again. If try is orange, which I expect, then I will disable failing tests in browser.ini. Try: https://tbpl.mozilla.org/?tree=Try&rev=cfab6511e30e https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cfab6511e30e

Comment on attachment 8515041 [details] [diff] [review] zero-passes-zero-fails-should-fail-1090913.patch Review of attachment 8515041 [details] [diff] [review]: ----------------------------------------------------------------- as a questions this is only in browser-test.js (browser-chrome/devtools). should we put this in simpletest.js so mochitest-plain and chrome can enjoy the benefits of it?

(In reply to Joel Maher (:jmaher) from comment #6) > Comment on attachment 8515041 [details] [diff] [review] > zero-passes-zero-fails-should-fail-1090913.patch > > Review of attachment 8515041 [details] [diff] [review]: > ----------------------------------------------------------------- > > as a questions this is only in browser-test.js (browser-chrome/devtools). > should we put this in simpletest.js so mochitest-plain and chrome can enjoy > the benefits of it? SimpleTest already does it... I guess it was just never implemented for browser chrome mochitests. I have copied the last part of the simpletest error message for consistency: "This test contains no passes, no fails and no todos. Maybe it threw a silent exception? Make sure you use waitForExplicitFinish() if you need it."

Fixed more tests that threw silent exceptions or were inappropriately disabled. Try: https://tbpl.mozilla.org/?tree=Try&rev=f5d0d3a3d3d7 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5d0d3a3d3d7

Now all tests pass so here are the changes: 1. Removed browser_newtab_reset.js and browser_newtab_tabsync.js as the features were never implemented. 2. Added a pass using ok(true, "...") to tests that rely on timeouts to detect failure. 3. When tests were disabled in the test file rather than browser.ini they are now disabled in browser.ini instead. 4. Fixed browser_audionode-actor-get-param-flags.js... for whatever reason using Array.forEach never worked (it never entered the loop) in that test so we use simpler for loops and a syntax error was also fixed. 5. browser_bug593535.js was accidentally landed with a return on the first line of test() so it never ran. I have removed the return, disabled the test via browser.ini and logged a bug to fix the leak.

Comment on attachment 8516755 [details] [diff] [review] zero-passes-zero-fails-should-fail-1090913.patch Review of attachment 8516755 [details] [diff] [review]: ----------------------------------------------------------------- 2 concerns with this patch, otherwise everything else is great! ::: browser/devtools/commandline/test/helpers.js @@ +1172,5 @@ > assert.log('Skipped ' + name + ' ' + skipReason); > + > + // Tests need at least one pass, fail or todo. Let's create a dummy pass > + // in case there are none. > + ok(true, "Each test requires at least one pass, fail or todo so here is a pass."); this is added in a helpers.js file, not a test file; is this a shortcut? ::: browser/devtools/tilt/test/head.js @@ +14,5 @@ > let LayoutHelpers = tempScope.LayoutHelpers; > > +// Tilt tests aborts early if it is not enabled or WebGL is not supported and we > +// need at least one pass, fail or todo so let's add a dummy pass. > +ok(true, "Each test requires at least one pass, fail or todo so here is a pass."); this looks like another shortcut, is it unrealistic to do this per test file?

(In reply to Joel Maher (:jmaher) from comment #11) > Comment on attachment 8516755 [details] [diff] [review] > zero-passes-zero-fails-should-fail-1090913.patch > > Review of attachment 8516755 [details] [diff] [review]: > ----------------------------------------------------------------- > > 2 concerns with this patch, otherwise everything else is great! > > ::: browser/devtools/commandline/test/helpers.js > @@ +1172,5 @@ > > assert.log('Skipped ' + name + ' ' + skipReason); > > + > > + // Tests need at least one pass, fail or todo. Let's create a dummy pass > > + // in case there are none. > > + ok(true, "Each test requires at least one pass, fail or todo so here is a pass."); > > this is added in a helpers.js file, not a test file; is this a shortcut? > GCLI tests contain an array of objects that look like this: ``` { skipRemainingIf: options.isNoDom, name: '|tsv option', setup: function() { ... }, check: { ... } }, { ... } ``` The only reason that the test is likely to exit before running any tests is if skipRemainingIf evaluates to true during the first test (it skips the current test and all remaining tests). Also, these tests are automatically generated as part of the GCLI project so changes to the test files themselves would be overwritten. Creating a pass here is the right thing to do. > ::: browser/devtools/tilt/test/head.js > @@ +14,5 @@ > > let LayoutHelpers = tempScope.LayoutHelpers; > > > > +// Tilt tests aborts early if it is not enabled or WebGL is not supported and we > > +// need at least one pass, fail or todo so let's add a dummy pass. > > +ok(true, "Each test requires at least one pass, fail or todo so here is a pass."); > > this looks like another shortcut, is it unrealistic to do this per test file? No, that really was a shortcut. I will move this into the test files as you suggest. I suppose we need a new try run: https://tbpl.mozilla.org/?tree=Try&rev=cea4b6b16c92 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cea4b6b16c92

Backed out as this patch somehow breaks test-pass and test-unexpected-fail messages in test logs: https://hg.mozilla.org/integration/fx-team/rev/7a7921e43070

Turned out to be build issues on Linux 64 stopping pass and fail messages and not this patch. Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/26b20a6c6080

Did you consider that many features get disabled through a pref, and thus some tests will check the pref and bail out? Those tests are going to fail when we move to Aurora, in a few days. I just hit this when disabling unifiedcomplete feature in Nightly, all of the tests that were bailing out started failing. I strongly suggest you do an m-c as Aurora Try build to check which tests may be affected. Also, please notify about this change in firefox-dev, mozilla.dev.platform and eventually a blog post on planet.