Closed
Bug 1495601
Opened 6 years ago
Closed 6 years ago
jstests fails if it's executed without virtualenv
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(1 file)
3.61 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
after bug 1305743, running jstests.py locally fails because it cannot find mozinfo.json. the following line fails because virtualenv is not created. https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/testing/mozbase/mozinfo/mozinfo/mozinfo.py#245 > try: > from mozbuild.base import MozbuildObject, BuildEnvironmentNotFoundException and it ends up raising error: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/testing/mozbase/mozinfo/mozinfo/mozinfo.py#267 > raise EnvironmentError('{}: could not find any mozinfo.json.'.format(__name__)) as far as I can see, the above import has been failing for a long time for local testing, but it was silently ignored. anyway, for local testing, the failure won't be fatal, and we should pass 'raise_exception'=False to suppress the fatal error.
Assignee | ||
Comment 1•6 years ago
|
||
for now I disabled the error on non-automation.
Updated•6 years ago
|
Attachment #9013463 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 2•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6568853848ac62384a094e521965e101978b1f62 Bug 1495601 - Do not raise error if jstests.py is executed without virtualenv on non-automation. r=sfink
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13309 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/13309 * continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/436017664?utm_source=github_status&utm_medium=notification)
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6568853848ac
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 6•6 years ago
|
||
This broke unit tests upstream.
Flags: needinfo?(lhansen)
Flags: needinfo?(james)
Comment 7•6 years ago
|
||
I don't know anything about the patch but this seems very related to the problem I ran into yesterday when testing on a system that was not also a build system, so preserving the functionality of the change would be welcome.
Flags: needinfo?(lhansen)
Assignee | ||
Comment 8•6 years ago
|
||
maybe testing/mozharness/mozinfo/mozinfo.py should also be updated? it complains that find_and_update_from_json doesn't accept keyword arguments, and what I can find is that another definition of find_and_update_from_json there, which I haven't hit while fixing this bug.
Assignee | ||
Comment 9•6 years ago
|
||
if the issue is from the outdated code in mozharness, bug 1372239 will fix it.
Comment 10•6 years ago
|
||
No, the problem is that Bug 1305743 changed mozinfo API and behaviour in a backwards-incompatible way. That behaviour change doesn't make sense in all cases (e.g. the case in this bug, or any other case where wpt is run against something other than a gecko built from source). So this bug changed the wpt harness to opt in to the old behaviour. That opt-in requires API surface that doesn't exist in the released mozinfo, and therefore breaks wpt on GitHub which is using the released version. I'm not sure what the best fix is; I tend to think that we should revert the change in Bug 1305743 and look for an approach that doesn't involve so many breaking changes.
Flags: needinfo?(james)
Flags: needinfo?(gbrown)
Flags: needinfo?(egao)
Flags: needinfo?(ahal)
Comment 11•6 years ago
|
||
Yeah, let's keep it backwards compatible. Rather than revert, we can just change the default of `raise_exception` to False: https://searchfox.org/mozilla-central/source/testing/mozbase/mozinfo/mozinfo/mozinfo.py#257
Flags: needinfo?(ahal)
Comment 12•6 years ago
|
||
I advocated for the incompatible change, reasoning that callers should not expect mozinfo.find_and_update_from_json(<dirs-without-mozinfo>) to "succeed". I'm hinting that failing callers should re-examine their use case, but in light of the trouble seen here, I agree we should change the default of raise_exception to False.
Flags: needinfo?(gbrown)
Comment 13•6 years ago
|
||
I agreed with the reasoning of :ahal and :gbrown when I refactored the behavior. However, whatever the intended result the actual result differed. I have reopened the Phabricator diff for this issue at https://phabricator.services.mozilla.com/D6860. If the call to kwargs cannot find a raise_exception key, it will now default to False. Unit tests failed when I made the change (as I would expect) and so its behavior was also changed to expect None to be raised by default.
Flags: needinfo?(egao)
Comment 14•6 years ago
|
||
This was backed out together with https://bugzilla.mozilla.org/show_bug.cgi?id=1305743#c17 as requested by jgraham.
Status: RESOLVED → REOPENED
status-firefox64:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
Comment 15•6 years ago
|
||
Backout by csabou@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/317b91c1fbd5 Backed out changeset 6568853848ac on request from jgraham for causing issues with mozinfo.json. a=backout
Upstream PR was closed without merging
Assignee | ||
Comment 17•6 years ago
|
||
closing given bug 1305743 is backed out and it works without any change now.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•