Closed Bug 1109183 Opened 5 years ago Closed 5 years ago

Marionette's test imports need to be reworked to correspond to a package, not a particular directory

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

(Keywords: pi-marionette-client)

Attachments

(1 file, 1 obsolete file)

ahal pointed out yesterday that my changes to mach_bootstrap.py in bug 1050511 broke some things and leaves us in a state where we're not actually importing from the package. All the test files assume that testing/marionette/client/marionette is on sys.path for their imports, so this was the simplest way to fix the issue in this bug.

I think the way forward for this will be to update all the imports to use the package, so essentially the imports will assume the tests are being run somewhere marionette is installed (and will work either because it is or because mach is hacking up an environment where it looks like that) instead of assuming they have access to everything in the testing/marionette/client/marionette directory.

I think the client and harness split in bug 1107336 will help this situation significantly, and may change some of my assumptions here, but I'll take a look at this once that lands.
For the record, this is breaking the b2g desktop reftest mach command (bug 1114474)
Depends on: 1107336
Does this actually depend on bug 1107336 or are we just trying to avoid bitrot? If it's the latter I think we should just go ahead and fix it, I've been pinged about the issue in bug 1114474 about 5-6 times now. If that is the case I don't mind doing the work :)
Flags: needinfo?(cmanchester)
I'm fairly sure bug 1107336 will fix this bug. I offered in bug 1114474 to back out the change that created this problem if the cure seemed worse than the disease in the interim. I actually wrote a patch to fix this before I realized the overlap with bug 1107336, but it involved moving runtests.py to its parent directory to resolve an ambiguity between marionettes which would mean mozharness and gaiatest modifications as well. Let me know if you want me to post that patch. I don't know how far off bug 1107336 is from landing, but I keep thinking it's soon.
Flags: needinfo?(cmanchester)
Ah, I didn't realize that bug was expected to fix this entirely. Also I just got a review request for it.. so I guess that answers that question :).
Need to look into this and possibly resolve now that bug 1107336 has landed.
Flags: needinfo?(cmanchester)
Some imports still need to be updated. Working on this now.
Flags: needinfo?(cmanchester)
/r/4435 - Bug 1109183 - Fix imports of the marionette client and remove spurious entry from sys.path provided by marionette.;r=ahal

Pull down this commit:

hg pull review -r 033f2a0561aa467cd857d4190585eede32bf0aff
Attachment #8570676 - Flags: review?(ahalberstadt)
Comment on attachment 8570676 [details]
MozReview Request: bz://1109183/chmanchester

https://reviewboard.mozilla.org/r/4433/#review3681

::: testing/marionette/client/marionette/tests/unit/test_typing.py
(Diff revision 1)
> -from marionette_test import MarionetteTestCase, skip_if_b2g
> +from marionette.marionette_test import MarionetteTestCase, skip_if_b2g

Shouldn't this just be "from marionette" ?

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> -from marionette_test import MarionetteTestCase, skip_if_e10s
> +from marionette.marionette_test import MarionetteTestCase, skip_if_e10s

Shouldn't this just be "from marionette" ?
Attachment #8570676 - Flags: review?(ahalberstadt)
https://reviewboard.mozilla.org/r/4433/#review3683

> Shouldn't this just be "from marionette" ?

skip_if* are importable from marionette_test, MarionetteTestCase is importable from both marionette and marionette_test.
Attachment #8570676 - Flags: review?(ahalberstadt)
Attachment #8570676 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8570676 [details]
MozReview Request: bz://1109183/chmanchester

https://reviewboard.mozilla.org/r/4433/#review3685

Ship It!
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
Attachment #8570676 - Attachment is obsolete: true
Comment on attachment 8572307 [details] [diff] [review]
Fix imports of the marionette client and remove spurious entry from sys.path provided by mach.

r=ahal
Attachment #8572307 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8add60e27b0a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.