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

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 1

[Andrew Halberstadt [:ahal]]

For the record, this is breaking the b2g desktop reftest mach command (bug 1114474)

Comment 2

[Andrew Halberstadt [:ahal]]

Also bug 1130922 (duped to bug 1114474)

Comment 3

[Andrew Halberstadt [:ahal]]

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 :)

Comment 4

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 5

[Andrew Halberstadt [:ahal]]

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 :).

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Need to look into this and possibly resolve now that bug 1107336 has landed.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Some imports still need to be updated. Working on this now.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1109183/chmanchester

/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

Comment 9

[Andrew Halberstadt [:ahal]]

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" ?

Comment 10

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8570676 [details]
MozReview Request: bz://1109183/chmanchester

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

Ship It!

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Attachment: Fix imports of the marionette client and remove spurious entry from sys.path provided by mach.

Patch to check in.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf28e611fbd7

Comment 13

[Chris Manchester (limited bugmail, email directly)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8add60e27b0a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8add60e27b0a

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Moving bugs for Marionette client due to component changes.