979640 - build system doesn't complain if a test is listed in mochitest.ini but doesn't actually exist

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The build system doesn't complain if a test is listed in mochitest.ini but the test file doesn't actually exist.

This is a pretty serious problem, since it means that if:
 * somebody makes a typo in the test name, or
 * there's a merge error due to directory moves, as actually happened in https://hg.mozilla.org/mozilla-central/rev/46df3fd9b0dc which was shortly after https://hg.mozilla.org/mozilla-central/rev/243259fda9ab
then we'll just end up with a test in the tree that's not being run, but looks like it's listed in a mochitest.ini.

This should be an error.

Comment 1

[Gregory Szorc [:gps]]

This is due to how we call active_tests():

            if filter_inactive:
                filtered = m.active_tests(disabled=False, **self.mozinfo)

active_tests has an "exists=True" argument that will only return tests that actually exist. I think we should force pass exists=False to active_tests().

Comment 2

[Gregory Szorc [:gps]]

Note that filter_inactivate is currently True for everything except xpcshell tests. This is because xpcshell tests are the only harness that is manifest driven.

Comment 3

[Gregory Szorc [:gps]]

This is high priority because it can lead to people thinking tests are enabled when they aren't.

Comment 4

[Gregory Szorc [:gps]]

Partial list of current failures:

/Users/gps/src/firefox/dom/tests/mochitest/webcomponents/mochitest.ini: test_dyanmic_content_element_matching.html, test_shadow_root.html, test_shadow_root_inert_element.html, test_shadow_root_style.html, test_shadow_root_style_multiple_shadow.html, test_shadow_root_style_order.html
/Users/gps/src/firefox/dom/imptests/html/mochitest.ini: html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-case.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-case.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-id.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-id.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-namespace.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-namespace.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-newelements.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-newelements.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-null-undef.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-null-undef.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-param.html, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-param.xhtml, html/dom/documents/dom-tree-accessors/document.getElementsByName/test_document.getElementsByName-same.html, html/dom/documents/dom-tree-accessors/test_Document.getElementsByClassName-null-undef.html, html/dom/documents/dom-tree-accessors/test_Element.getElementsByClassName-null-undef.html, html/dom/documents/dom-tree-accessors/test_document.body-getter.html, html/dom/documents/dom-tree-accessors/test_document.body-setter-01.html, html/dom/documents/dom-tree-accessors/test_document.embeds-document.plugins-01.html, html/dom/documents/dom-tree-accessors/test_document.getElementsByClassName-same.html, html/dom/documents/dom-tree-accessors/test_document.head-01.html, html/dom/documents/dom-tree-accessors/test_document.head-02.html, html/dom/documents/dom-tree-accessors/test_document.images.html, html/dom/documents/dom-tree-accessors/test_document.title-01.html, html/dom/documents/dom-tree-accessors/test_document.title-02.xhtml, html/dom/documents/dom-tree-accessors/test_document.title-03.html, html/dom/documents/dom-tree-accessors/test_document.title-04.xhtml, html/dom/documents/dom-tree-accessors/test_document.title-05.html, html/dom/documents/dom-tree-accessors/test_document.title-06.html, html/dom/documents/dom-tree-accessors/test_document.title-07.html, html/dom/documents/dom-tree-accessors/test_nameditem-01.html, html/dom/documents/dom-tree-accessors/test_nameditem-02.html, html/dom/documents/dom-tree-accessors/test_nameditem-03.html, html/dom/documents/dom-tree-accessors/test_nameditem-04.html, html/dom/documents/dom-tree-accessors/test_nameditem-05.html, html/dom/documents/dom-tree-accessors/test_nameditem-06.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_document-color-01.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_document-color-02.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_document-color-03.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_document-color-04.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_heading-obsolete-attributes-01.html, html/obsolete/requirements-for-implementations/other-elements-attributes-and-apis/test_script-IDL-event-htmlfor.html
/Users/gps/src/firefox/browser/devtools/commandline/test/browser.ini: browser_gcli_cli.js, browser_gcli_completion.js
/Users/gps/src/firefox/browser/devtools/sourceeditor/test/browser.ini: browser_sourceeditor_initialization.js
/Users/gps/src/firefox/browser/components/sessionstore/test/browser.ini: browser_346337.js

Comment 5

[Gregory Szorc [:gps]]

Attachment: Make build system error on missing test files

Previously, the build system may silently missing test files defined in
manifests. This patch makes missing test files a fatal error, detected
when reading test manifests.

The test_bug872273.html XBL test appeared to be orphaned in
content/xbl/test. It has been reunited with its family.

dom/tests/mochitest/notification referenced a single test file which was
recently deleted. That manifest has been removed.

Missing test files related to the Python unit tests for the build system
have been added. (They are a bunch of empty files.)

Comment 6

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=863057a116ae

Patch is only tested on OS X desktop build. I would be shocked if there weren't failures on other platforms.

Comment 7

[Gregory Szorc [:gps]]

For the record, I'm shocked by how green that try push has been so far.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Make build system error on missing test files

I rebased on top of central and there was a typo in a accessible -
s/text/test/.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8385855 [details] [diff] [review]
Make build system error on missing test files

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +474,5 @@
>              filtered = m.tests
>  
>              if filter_inactive:
> +                # We return tests that don't exist because we want manifests
> +                # defining tests that don't exist to result in error.

I feel like we should push this fix into manifestdestiny. File a followup? It should be hard to do the wrong thing with this API.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/593206fbd97e

(inbound was closed)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/fx-team/rev/d2066176c6f8

https://tbpl.mozilla.org/php/getParsedLog.php?id=35670619&tree=Fx-Team

Comment 12

[Gregory Szorc [:gps]]

I disabled the test and re-landed:

https://hg.mozilla.org/integration/fx-team/rev/5e6934522047

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Gregory Szorc [:gps] from comment #12)
> I disabled the test and re-landed:

This test was recently added. Please file a bug and assign to cabanier?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e6934522047