1383221 - Tests that exist in tree but not defined in any manifests

Status: RESOLVED WORKSFORME

(Testing :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

I was making some changes to some test related eslint helpers and had a bug. All of a sudden there were a bunch of eslint errors (mostly undefined vars). I did some digging, and it turns out they were all from tests that are in-tree but not listed in any manifests (and are therefore not running in ci)!

The following tests are all not running in ci:
browser/extensions/shield-recipe-client/test/unit/test_ActionSandboxManager.js
browser/extensions/shield-recipe-client/test/unit/test_Utils.js
devtools/client/inspector/rules/test/browser_rules_css-docs-tooltip_closes-on-escape.js
devtools/client/inspector/test/browser_inspector_search-suggests-ids-and-classes.js
devtools/server/tests/unit/test_profiler_bufferstatus.js
toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_extension_content_telemetry.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js
toolkit/components/places/tests/history/test_hasVisits.js
toolkit/components/places/tests/unit/test_adaptive.js

Comment 1

[Andrew Halberstadt [:ahal]]

Hi everyone, you are being needinfo'ed because you either authored, (most) recently modified or reviewed one or more of the tests above. It looks like these tests are all unlisted in any manifests, and therefore are not running in CI.

Please take a minute to triage your respective test(s) and file a follow-up blocking this. You can either:

A) Add the test to a manifest (and fix it if needed)
B) Add the test to a manifest and disable it
C) Delete the test

Tests can be disabled in a manifest like this:

[test_ActionSandboxManager.js]
disabled = too many intermittents (bug 1234567)

If you think you have been needinfo'ed in error, let me know.
Thanks!

p.s I'm also filing a follow up to add some kind of lint check to prevent tests like these from happening in the future.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1383221 - enable unused inspector search mochitest;

This mochitest was never added to browser.ini, and never ran on try.
I tested it locally and the test still seems to run. However the mdn
docs widget is now disabled by default and is actually not working
very well when enabled.

I propose to remove the file rather than slowing down our test suite
for this.

Review commit: https://reviewboard.mozilla.org/r/159954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159954/

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the heads up.

For devtools/client/inspector/rules/test/browser_rules_css-docs-tooltip_closes-on-escape.js, I propose to delete the test. The widget is disabled by default, probably soon to be removed completely, can't see the point of enabling now.

Comment 4

[Julian Descottes [:jdescottes]]

pbro is on PTO so answering on his behalf about devtools/client/inspector/test/browser_inspector_search-suggests-ids-and-classes.js 

This test was not enabled for a long time (at least back to the migration from toolkit/ to devtools/), but it does test some features not tested by any other inspector search test. Quickly fixed the test, let's see if we can get a green try https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ab48cb6e8a410b93c1b44a0896bce8bb9d1a32

Comment 5

[Osmose [:osmose, :mkelly]]

PR for the Shield tests is up at https://github.com/mozilla/normandy/pull/904. I can create a Bugzilla bug to track it as well if you really want; that PR will land and hit mozilla-central next time we do a sync from Github in either case.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8888923 [details]
Bug 1383221 - enable unused inspector search mochitest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159954/diff/1-2/

Comment 7

[Marco Bonardo [:mak]]

Filed bug 1383639 for the Places tests. thank you!

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8888923 [details]
Bug 1383221 - enable unused inspector search mochitest;

https://reviewboard.mozilla.org/r/159954/#review165702

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8888923 [details]
Bug 1383221 - enable unused inspector search mochitest;

https://reviewboard.mozilla.org/r/159954/#review165704

::: commit-message-7d2e8:1
(Diff revision 2)
> +Bug 1383221 - enable unused inspector search mochitest;r=gl

I think we can change the commit message to simply state enable browser_inspector_search-suggests-ids-and-classes.js mochitest to clarify things a bit.

Comment 10

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1383221 - Add test_ext_browserSettings.js back into toolkit/components/extensions/test/xpcshell/xpcshell-common.ini,

This was apparently removed from the ini files in https://hg.mozilla.org/mozilla-central/rev/9704283b0e3a and never added back.

Review commit: https://reviewboard.mozilla.org/r/160468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160468/

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8889420 [details]
Bug 1383221 - Add test_ext_browserSettings.js back into toolkit/components/extensions/test/xpcshell/xpcshell-common.ini,

I will open a separate bug for this as there are three tests that need to be addressed and a couple of them are failing locally for me.

Comment 12

[Bob Silverberg [:bsilverberg]]

I filed bug 1383728 for the WebExtensions tests.

Comment 13

[Andrew Halberstadt [:ahal]]

Thanks for all the quick responses!

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #5)
> PR for the Shield tests is up at
> https://github.com/mozilla/normandy/pull/904. I can create a Bugzilla bug to
> track it as well if you really want; that PR will land and hit
> mozilla-central next time we do a sync from Github in either case.

I don't think a bug's necessary

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/1addd817476d382d77f57dcdff12624603dbbe60
Bug 1383221: Add tests that weren't being run to manifests.

ActionSandboxManager.jsm imports ShellService.jsm, which is not supported by
xpcshell, so it had to be ported to a mochitest.

https://github.com/mozilla/normandy/commit/b6c410ed4a76823e07cc826b102b8b3f3f6ac139
Merge pull request #904 from Osmose/missing-tests

Bug 1383221: Add tests that weren't being run to manifests.

Comment 15

[Julian Descottes [:jdescottes]]

For the record, Bug 1383723 addressed:
- devtools/client/inspector/rules/test/browser_rules_css-docs-tooltip_closes-on-escape.js
- devtools/client/inspector/test/browser_inspector_search-suggests-ids-and-classes.js

Comment 16

[Bob Silverberg [:bsilverberg]]

Bug 1383728 landed, which addresses:

toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_extension_content_telemetry.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js

Comment 17

[Andrew Halberstadt [:ahal]]

Looks like these all got picked up, thanks everyone.

Comment 18

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.