1305743 - Make failure to find mozinfo.json a fatal error

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Gregory Szorc [:gps]]

In bug 1296397, I ran into issues where WPT was running in automation without a loaded mozinfo.json because the path to mozinfo.json changed. This caused a number of head-scratching test failures because it wasn't obvious that mozinfo.json wasn't loaded.

The code in mozinfo.find_and_update_from_json() currently no-ops if no mozinfo.json could be loaded. Since mozinfo.json is kinda critical to ensure tests run properly, we should consider changing it to fail if it can't find a mozinfo.json.

Alternatively, callers of this function relying on a mozinfo.json (such as automation) should raise if it returns None.

Comment 1

[Edwin Takahashi (:egao | infrequent contributor)]

Try run of a proposed change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f50358b5e5c3133895478cbc5c197575850b3637

Comment 2

[Edwin Takahashi (:egao | infrequent contributor)]

Changes are causing two jobs to fail:

- linux x64 opt python 2 unit tests source-test-python-mozbase-py2 py2(mb).
- linux x64 opt python 3 unit tests source-test-python-mozbase-py3 py3(mb).

Unsure if this is the intended side effect of raising an explicit error instead of no-oping. An alternative explanation is that by default my MOZCONFIG contains --artifact flag, so these builds fail since it is expecting a full build to be performed prior to running the test.

:gbrown: What do you think? Is a more extensive try run needed to gauge further possible impact on test suite?

Comment 3

[Geoff Brown [:gbrown]]

I don't think your MOZCONFIG would have an effect here.

Those failures look to be a result of your change:

https://treeherder.mozilla.org/logviewer.html#?job_id=201450529&repo=try&lineNumber=317-328

[task 2018-09-25T16:14:34.319Z] Traceback (most recent call last):
[task 2018-09-25T16:14:34.319Z]   File "/builds/worker/checkouts/gecko/build/gen_test_backend.py", line 39, in <module>
[task 2018-09-25T16:14:34.319Z]     sys.exit(gen_test_backend())
[task 2018-09-25T16:14:34.319Z]   File "/builds/worker/checkouts/gecko/build/gen_test_backend.py", line 30, in gen_test_backend
[task 2018-09-25T16:14:34.319Z]     emitter = TreeMetadataEmitter(config)
[task 2018-09-25T16:14:34.319Z]   File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 114, in __init__
[task 2018-09-25T16:14:34.319Z]     mozinfo.find_and_update_from_json(config.topobjdir)
[task 2018-09-25T16:14:34.319Z]   File "/builds/worker/checkouts/gecko/testing/mozbase/mozinfo/mozinfo/mozinfo.py", line 266, in find_and_update_from_json
[task 2018-09-25T16:14:34.319Z]     raise IOError
[task 2018-09-25T16:14:34.319Z] IOError
[task 2018-09-25T16:14:34.332Z] /builds/worker/checkouts/gecko/build/rebuild-backend.mk:24: recipe for target 'backend.TestManifestBackend' failed
[task 2018-09-25T16:14:34.332Z] make: *** [backend.TestManifestBackend] Error 1

Does that mean that this find_and_update_from_json call normally no-ops in this test? Maybe you should re-run py2/3(mb) on try without your change but with some additional logging to verify.

Comment 4

[Andrew Halberstadt [:ahal]]

Mozinfo was sort of designed to be unaware of things like "objdir" and "mozilla-central". It's definitely used by things that live outside of the tree as well. So the best place to fix this is likely in the test harnesses (e.g the 'update_mozinfo' function in mochitest).

The downside to that location is that we have to implement this for every test harness though (and new harnesses might omit this). Maybe the `find_and_update_from_json` function could take a raises parameter (which consumers that want an error can set).

Comment 5

[Edwin Takahashi (:egao | infrequent contributor)]

After running through several different combinations locally and on try-server:

- replacing the | return None | call with | raise <Exception> | will cause py2/py3(mb) tests to fail on try-server.
- tested locally, the above change does not cause test failure in any scenario.

The failure location on try-server is in the test setup phase:

Build configuration changed. Regenerating backend.
[task 2018-09-25T20:24:43.302Z] No build detected, test metadata may be incomplete.
[task 2018-09-25T20:24:43.302Z] paths is: [u'/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/mozinfo.json']
[task 2018-09-25T20:24:43.302Z] path has mozinfo: [False]
[task 2018-09-25T20:24:43.302Z] valid files: []

In the above snippet, debug messages I added are visible.


The following is a snippet from a try run where only debug messages were added:

[task 2018-09-25T19:26:04.428Z] Build configuration changed. Regenerating backend.
[task 2018-09-25T19:26:04.910Z] No handlers could be found for logger "mozbuild.frontend.reader"
[task 2018-09-25T19:26:08.782Z] No build detected, test metadata may be incomplete.
[task 2018-09-25T19:26:08.782Z] now in the try for the objdir path
[task 2018-09-25T19:26:08.782Z] has it found build?: <mozbuild.base.MozbuildObject object at 0x7f4f0e4f8ed0>
[task 2018-09-25T19:26:08.782Z] dirs value is: (u'/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu',)
[task 2018-09-25T19:26:08.782Z] d is: /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu


---

So from what I gather, mozbase tests rely on the fact that mozinfo.find_and_update_from_json() no-ops.

With regard to :ahal:, I agree that having harness maintainers handle it is not a good approach. Ideal scenario is to fix behavior of find_and_update_from_json(). 

One complicating factor in the parameter approach is that Python2.7 does not support mixing of keyword arguments with defaults after positional arguments. See https://stackoverflow.com/questions/5940180/python-default-keyword-arguments-after-variable-length-positional-arguments/5940226.

Ideally the signature of the function is not modified, but since Python2.7 is still current for us this may require an approach that may prove brittle - to use a special keyword argument in the positional argument, e.g.:

mozinfo.find_and_update_from_json('path1', 'path2', True)

Where the True boolean maps to an internal raise_exception variable.

Comment 6

[Edwin Takahashi (:egao | infrequent contributor)]

Small try run of a proposed fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67693fe4b0bd0dcb88a97a0a47630191dd90c74c

Note the python2/3(mb) tests are now passing.

Proposed patch maintains signature of the original method, but adds ability for caller to toggle if exceptions are desired.

---

Full try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56892625d0b032fc64241980ce8f1efc726a57e9

Comment 7

[Edwin Takahashi (:egao | infrequent contributor)]

Full try run of a newer version of the patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c910f5a725aebf95292a2d31e8946003ffa3e4ad

Comment 8

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1305743 - Make failure to find mozinfo.json a fatal error r?gbrown,ahal

- added codepath prior to main portion to check if caller has passed in an optional raise_exception boolean value.
- if boolean value is detected, function enters new codepath which raises an exception.
- if boolean value is not detected, function continues to behave as it did prior to the patch.

Comment 9

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1305743 - Add optional keyword argument raise_exception to mozinfo.find_and_update_from_json r?gbrown,ahal

- changed placement of the raise_exception logic to be after the initial objdir path.
- original implementation was missing detection for False cases, changed logic to detect such cases.
- added unit tests to check for scenarios where raise_exception flag is passed in as part of positional argument.

Depends on D6859

Comment 10

[Geoff Brown [:gbrown]]

I think the value in this bug is in avoiding the frustration expressed in comment 0: A current or future client who calls find_and_update_from_json(path) should be able to expect something bad to happen if path does not exist or is otherwise invalid. 

If using this raise_exception parameter, I think it should be True by default: An exception is raised unless the caller explicitly requests that exceptions are suppressed. But actually, I'd prefer that find_and_update_from_json() always raised an exception on failure, and we rely on callers to try/catch where needed:

try:
    find_and_update_from_json(path)
except IOError:
    log.info("I tried to update mozinfo from %s, but that was not found...but that is okay in this case" % path)

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 9011983 [details]
Bug 1305743 - Add optional keyword argument raise_exception to mozinfo.find_and_update_from_json r?gbrown,ahal

Andrew Halberstadt [:ahal] has approved the revision.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 9011983 [details]
Bug 1305743 - Add optional keyword argument raise_exception to mozinfo.find_and_update_from_json r?gbrown,ahal

Geoff Brown [:gbrown] has approved the revision.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d25c44c39ea
Make failure to find mozinfo.json a fatal error r=gbrown,ahal

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6d25c44c39ea

Comment 15

[Edwin Takahashi (:egao | infrequent contributor)]

Reopened issue due to issues caused downstream by this change.

New Phabricator diff is up that addresses concerns raised against changing behavior of mozinfo.find_and_update_by_json().

Changes:

- default to pre-patch behavior, where errors are suppressed. To raise an explicit error, user must pass an optional value 'raise_exception'.

Comment 16

[James Graham [:jgraham]]

I think this also caused bug 1496141

Comment 17

[Cosmin Sabou [:CosminS]]

Backed out on request from jgraham for causing issues with mozinfo.json

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8660ad891a23c389512aa8d9d1fb122096df6741&searchStr=linux,x64,opt,python,2,unit,tests,source-test-python-marionette-harness-py2,py2(mnh)&selectedJob=203128422

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=203128422&repo=autoland&lineNumber=575

Backout link: https://hg.mozilla.org/mozilla-central/rev/7a00217bd2fac77aaad3d3a98730594b362ede5f

[task 2018-10-03T15:59:14.146Z]  3:33.14 TEST-START | testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py::test_add_test_manifest[enabled-True]
[task 2018-10-03T15:59:14.146Z]  3:33.14 TEST-UNEXPECTED-FAIL | testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py::test_add_test_manifest[enabled-True] | EnvironmentError: mozinfo.mozinfo: could not find any mozinfo.json. (line 267)
[task 2018-10-03T15:59:14.146Z]  3:33.14 mock_runner = <marionette_harness.runtests.MarionetteTestRunner object at 0x7f831881f050>
[task 2018-10-03T15:59:14.146Z]  3:33.14 manifest_with_tests = <ManifestFixture enabled>
[task 2018-10-03T15:59:14.146Z]  3:33.14 monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f831881f910>
[task 2018-10-03T15:59:14.146Z]  3:33.14 test_files_exist = True
[task 2018-10-03T15:59:14.146Z]  3:33.14 
[task 2018-10-03T15:59:14.146Z]  3:33.14     @pytest.mark.parametrize("test_files_exist", [True, False])
[task 2018-10-03T15:59:14.146Z]  3:33.14     def test_add_test_manifest(mock_runner, manifest_with_tests, monkeypatch, test_files_exist):
[task 2018-10-03T15:59:14.146Z]  3:33.14         monkeypatch.setattr('marionette_harness.runner.base.TestManifest',
[task 2018-10-03T15:59:14.146Z]  3:33.14                             manifest_with_tests.manifest_class)
[task 2018-10-03T15:59:14.146Z]  3:33.14         mock_runner.marionette = mock_runner.driverclass()
[task 2018-10-03T15:59:14.146Z]  3:33.14         with patch('marionette_harness.runner.base.os.path.exists', return_value=test_files_exist):
[task 2018-10-03T15:59:14.146Z]  3:33.14             if test_files_exist or manifest_with_tests.n_enabled == 0:
[task 2018-10-03T15:59:14.146Z]  3:33.14 >               mock_runner.add_test(manifest_with_tests.filepath)
[task 2018-10-03T15:59:14.146Z]  3:33.14 
[task 2018-10-03T15:59:14.146Z]  3:33.14 testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py:310:
[task 2018-10-03T15:59:14.146Z]  3:33.14 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[task 2018-10-03T15:59:14.146Z]  3:33.14 testing/marionette/harness/marionette_harness/runner/base.py:994: in add_test
[task 2018-10-03T15:59:14.146Z]  3:33.14     json_path = update_mozinfo(filepath)
[task 2018-10-03T15:59:14.147Z]  3:33.14 testing/marionette/harness/marionette_harness/runner/base.py:50: in update_mozinfo
[task 2018-10-03T15:59:14.147Z]  3:33.14     return mozinfo.find_and_update_from_json(*dirs)
[task 2018-10-03T15:59:14.147Z]  3:33.14 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[task 2018-10-03T15:59:14.147Z]  3:33.14 
[task 2018-10-03T15:59:14.147Z]  3:33.14 dirs = ('/path/to/fake/manifest.ini', '/path/to', '/path/to/fake', '/', '/path')
[task 2018-10-03T15:59:14.147Z]  3:33.14 kwargs = {}, MozbuildObject = <class 'mozbuild.base.MozbuildObject'>
[task 2018-10-03T15:59:14.147Z]  3:33.14 BuildEnvironmentNotFoundException = <class 'mozbuild.base.BuildEnvironmentNotFoundException'>
[task 2018-10-03T15:59:14.147Z]  3:33.14 MozconfigFindException = <class 'mozbuild.mozconfig.MozconfigFindException'>
[task 2018-10-03T15:59:14.148Z]  3:33.14 build = <mozbuild.base.MozbuildObject object at 0x7f8318832310>
[task 2018-10-03T15:59:14.148Z]  3:33.14 json_path = '/path/mozinfo.json', raise_exception = True, d = '/path'
[task 2018-10-03T15:59:14.148Z]  3:33.14 
[task 2018-10-03T15:59:14.148Z]  3:33.14     def find_and_update_from_json(*dirs, **kwargs):
[task 2018-10-03T15:59:14.148Z]  3:33.14         """Find a mozinfo.json file, load it, and update global symbol table.
[task 2018-10-03T15:59:14.148Z]  3:33.14 
[task 2018-10-03T15:59:14.149Z]  3:33.14         This method will first check the relevant objdir directory for the
[task 2018-10-03T15:59:14.149Z]  3:33.14         necessary mozinfo.json file, if the current script is being run from a
[task 2018-10-03T15:59:14.149Z]  3:33.14         Mozilla objdir.
[task 2018-10-03T15:59:14.149Z]  3:33.14 
[task 2018-10-03T15:59:14.150Z]  3:33.14         If the objdir directory did not supply the necessary data, this method
[task 2018-10-03T15:59:14.150Z]  3:33.14         will then look for the required mozinfo.json file from the provided
[task 2018-10-03T15:59:14.150Z]  3:33.14         tuple of directories.
[task 2018-10-03T15:59:14.150Z]  3:33.14 
[task 2018-10-03T15:59:14.151Z]  3:33.14         If file is found, the global symbols table is updated via a helper method.
[task 2018-10-03T15:59:14.151Z]  3:33.14 
[task 2018-10-03T15:59:14.152Z]  3:33.14         If no valid files are found, an exception is raised.
[task 2018-10-03T15:59:14.152Z]  3:33.14 
[task 2018-10-03T15:59:14.152Z]  3:33.14         :param tuple dirs: Directories in which to look for the file.
[task 2018-10-03T15:59:14.153Z]  3:33.14         :param dict kwargs: optional values:
[task 2018-10-03T15:59:14.153Z]  3:33.14                             raise_exception: if this value is provided, the default
[task 2018-10-03T15:59:14.153Z]  3:33.14                                              behavior of raising an exception is
[task 2018-10-03T15:59:14.153Z]  3:33.14                                              overridden.
[task 2018-10-03T15:59:14.154Z]  3:33.14         :returns: EnvironmentError: default behavior.
[task 2018-10-03T15:59:14.155Z]  3:33.14                   None: if exception raising is suppressed.
[task 2018-10-03T15:59:14.155Z]  3:33.14                   json_path: string representation of path.
[task 2018-10-03T15:59:14.155Z]  3:33.14         """
[task 2018-10-03T15:59:14.156Z]  3:33.14         # First, see if we're in an objdir
[task 2018-10-03T15:59:14.156Z]  3:33.14         try:
[task 2018-10-03T15:59:14.156Z]  3:33.14             from mozbuild.base import MozbuildObject, BuildEnvironmentNotFoundException
[task 2018-10-03T15:59:14.156Z]  3:33.14             from mozbuild.mozconfig import MozconfigFindException
[task 2018-10-03T15:59:14.157Z]  3:33.14             build = MozbuildObject.from_environment()
[task 2018-10-03T15:59:14.157Z]  3:33.15             json_path = _os.path.join(build.topobjdir, "mozinfo.json")
[task 2018-10-03T15:59:14.157Z]  3:33.15             if _os.path.isfile(json_path):
[task 2018-10-03T15:59:14.157Z]  3:33.15                 update(json_path)
[task 2018-10-03T15:59:14.157Z]  3:33.15                 return json_path
[task 2018-10-03T15:59:14.158Z]  3:33.15         except ImportError:
[task 2018-10-03T15:59:14.158Z]  3:33.15             pass
[task 2018-10-03T15:59:14.158Z]  3:33.15         except (BuildEnvironmentNotFoundException, MozconfigFindException):
[task 2018-10-03T15:59:14.158Z]  3:33.15             pass
[task 2018-10-03T15:59:14.158Z]  3:33.15 
[task 2018-10-03T15:59:14.158Z]  3:33.15         raise_exception = kwargs.get('raise_exception', True)
[task 2018-10-03T15:59:14.159Z]  3:33.15 
[task 2018-10-03T15:59:14.159Z]  3:33.15         for d in dirs:
[task 2018-10-03T15:59:14.159Z]  3:33.15             d = _os.path.abspath(d)
[task 2018-10-03T15:59:14.159Z]  3:33.15             json_path = _os.path.join(d, "mozinfo.json")
[task 2018-10-03T15:59:14.159Z]  3:33.15             if _os.path.isfile(json_path):
[task 2018-10-03T15:59:14.159Z]  3:33.15                 update(json_path)
[task 2018-10-03T15:59:14.159Z]  3:33.15                 return json_path
[task 2018-10-03T15:59:14.160Z]  3:33.15 
[task 2018-10-03T15:59:14.160Z]  3:33.15         if raise_exception:
[task 2018-10-03T15:59:14.161Z]  3:33.15 >           raise EnvironmentError('{}: could not find any mozinfo.json.'.format(__name__))
[task 2018-10-03T15:59:14.161Z]  3:33.15 E           EnvironmentError: mozinfo.mozinfo: could not find any mozinfo.json.
[task 2018-10-03T15:59:14.161Z]  3:33.15 
[task 2018-10-03T15:59:14.161Z]  3:33.15 testing/mozbase/mozinfo/mozinfo/mozinfo.py:267: EnvironmentError
[task 2018-10-03T15:59:14.161Z]  3:33.15 TEST-INFO took 115ms
[task 2018-10-03T15:59:14.161Z]  3:33.15 TEST-START | testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py::test_add_test_manifest[enabled-False]
[task 2018-10-03T15:59:14.161Z]  3:33.15 TEST-UNEXPECTED-FAIL | testing/marionette/harness/marionette_harness/tests/harness_unit/test_marionette_runner.py::test_add_test_manifest[enabled-False] | EnvironmentError: mozinfo.mozinfo: could not find any mozinfo.json. (line 267)
[task 2018-10-03T15:59:14.162Z]  3:33.15 mock_runner = <marionette_harness.runtests.MarionetteTestRunner object at 0x7f831892ec10>

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/dcba2a476ccf
Make failure to find mozinfo.json a fatal error r=gbrown,ahal a=reland

Comment 19

[Edwin Takahashi (:egao | infrequent contributor)]

:jgraham Would it be a viable solution for me to fix the way mozinfo.find_and_update_from_json() is being called with the patch applied? The original bug description states to remove the no-op behavior as this is/was causing other silent failures.

:CosminS Same applies here - would it be an option for me to fix the unit test that was failing in https://bugzilla.mozilla.org/show_bug.cgi?id=1496141?

Comment 20

[Cosmin Sabou [:CosminS]]

:egao, from what I spoke with jgraham on IRC we agreed that backing out this bug and bug 1495601 would solve the problem for now until you or somebody else will find a fix for this in a different (non-breaking) way. That bug there 1496141 will no longer fail after the backout.

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/520bcd79bd20
Backed out changeset dcba2a476ccf on request from jgraham for causing issues with mozinfo.json. a=backout

Comment 22

[James Graham [:jgraham]]

I think the right fix would be to make the behaviour opt-in, since there are many cases where not finding a mozinfo file isn't a problem.

Comment 23

[Geoff Brown [:gbrown]]

:egao - What is your plan here? Under the circumstances, I think opt-in would be okay. I could also see closing this as wontfix.

Comment 24

[Edwin Takahashi (:egao | infrequent contributor)]

(In reply to Geoff Brown [:gbrown] from comment #23)
> :egao - What is your plan here? Under the circumstances, I think opt-in
> would be okay. I could also see closing this as wontfix.

I will take another stab at fixing this without impacting current automation/processes, now that I have some more experience under my belt.

Comment 25

[Edwin Takahashi (:egao | infrequent contributor)]

Newly attempted solution closely mirrors the last attempt in Phabricator diff D6860.

Preliminary Try run to validate build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bf43aaccc3e7e382509aefa743fb2f4eadd8a3e

- the preliminary try run is coming back all green.

Handpicked sets of tests to check changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fac3634ca430ba6163983dc827d44de53f8ac4f8

- tests do not appear to be affected by the changes to mozinfo.

Python2/3 unit tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0163edabad7eb10e64f95940d9ef5706b02b97b5

- python unit tests complete, no failures.

Comment 26

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1305743 - Add optional parameter to make failure to find mozinfo.json a fatal error r?gbrown

Changes:
- added optional keyword argument to the find_and_update_from_json() function: raise_exception
- the optional keyword argument accepts a boolean value depending on if exception is desired.

Tests
- added a few tests to ensure the expected exceptions are raised only when raise_exception flag is True.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4fe963f4eb4
Add optional parameter to make failure to find mozinfo.json a fatal error r=gbrown

Comment 28

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/c4fe963f4eb4