Closed Bug 1316707 Opened 3 years ago Closed 3 years ago

Remove B2G code from Marionette client and harness

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(5 files)

As part of the work for bug 1306604 this bug will remove all the B2G related code from the client and harness.
As given by bug 1315506 comment 5 we will have to keep the Marionette B2G update tests.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> As given by bug 1315506 comment 5 we will have to keep the Marionette B2G
> update tests.

Rob, given that we have to keep those tests I see problems in removing the B2G related code from Marionette. You are saying that the community is still using those tests? How is this different from all the other code pieces we are removing across the tree. I would like to understand why we really have to keep those tests in the tree.
Flags: needinfo?(robert.strong.bugs)
I have no problem with it being removed so go for it. I talked with a couple of community members awhile back and they just asked for best effort which is all I am doing. Go ahead and talk with community members if you have further questions along those lines.
Flags: needinfo?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #3)
> of community members awhile back and they just asked for best effort which
> is all I am doing. Go ahead and talk with community members if you have
> further questions along those lines.

Yes, but we will have to get this removed. As sad as it is. But there is always a way to checkout those tests again via m-c.

David, I wonder what we want to do with the Marionette JS Testcase class and mixins which are actually not in use by any code in m-c. Should we get this also completely removed?
Flags: needinfo?(dburns)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #3)
> > of community members awhile back and they just asked for best effort which
> > is all I am doing. Go ahead and talk with community members if you have
> > further questions along those lines.
> 
> Yes, but we will have to get this removed. As sad as it is. But there is
> always a way to checkout those tests again via m-c.

Do note that the Python related B2G code has not been in use for a very long time.  B2G migrated to MarionetteJS Gaia tests, which themselves communicate directly with the Marionette server so there should be not any danger in removing the B2G related code in the Python client and harness.

> David, I wonder what we want to do with the Marionette JS Testcase class and
> mixins which are actually not in use by any code in m-c. Should we get this
> also completely removed?

This should be safe to remove.
Flags: needinfo?(dburns)
(In reply to Andreas Tolfsen ‹:ato› from comment #10)
> > David, I wonder what we want to do with the Marionette JS Testcase class and
> > mixins which are actually not in use by any code in m-c. Should we get this
> > also completely removed?
> 
> This should be safe to remove.

Ok, just to be sure lets also ask Dave and Eric if he is aware of any code outside of the tree which might use those classes by just relying on the Marionette client package on pypi.

The mixins I'm talking about here are endurance.py, reporting.py:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/mixins
Flags: needinfo?(erahm)
Flags: needinfo?(dave.hunt)
I'm not aware of anything still using this code.
Flags: needinfo?(dave.hunt)
AWSY doesn't support b2g, so we're probably fine there.
Flags: needinfo?(erahm)
Attachment #8809589 - Flags: review?(dburns)
Attachment #8809590 - Flags: review?(dburns)
Attachment #8809591 - Flags: review?(dburns)
Attachment #8809592 - Flags: review?(dburns)
Attachment #8809593 - Flags: review?(dburns)
Attachment #8809590 - Flags: review?(dburns) → review?(robert.strong.bugs)
Comment on attachment 8809590 [details]
Bug 1316707 - Remove Marionette B2G update tests.

https://reviewboard.mozilla.org/r/92130/#review92762

I don't know this code so if there is anyone that has spent more time on it than I have you should ask them. From what little I know of it this should be fine.
Attachment #8809590 - Flags: review?(robert.strong.bugs) → review+
(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #3)
> > of community members awhile back and they just asked for best effort which
> > is all I am doing. Go ahead and talk with community members if you have
> > further questions along those lines.
> 
> Yes, but we will have to get this removed. As sad as it is. But there is
> always a way to checkout those tests again via m-c.
Not sure what you are trying to get at but like I said, "I have no problem with it being removed so go for it."
Attachment #8809589 - Flags: review?(dburns) → review?(ato)
Attachment #8809591 - Flags: review?(dburns) → review?(ato)
Attachment #8809592 - Flags: review?(dburns) → review?(ato)
Attachment #8809593 - Flags: review?(dburns) → review?(ato)
Comment on attachment 8809589 [details]
Bug 1316707 - Remove Marionette unit tests for B2G.

https://reviewboard.mozilla.org/r/92128/#review93768

I wonder if test_element_touch.py, test_gesture.py, test_multi_finger.py, and test_single_finger.py will be relevant for Fennec.  It is possible that @maja_zf envisions implementing the W3C WebDriver conformant actions for Android directly, in which case I guess it’s fine to delete these files.  Will you ping her first to check?

Anyway, deleting isn’t a problem as we can always dig up the history in revision control.
Attachment #8809589 - Flags: review?(ato) → review+
Comment on attachment 8809591 [details]
Bug 1316707 - Remove B2G mach command for Marionette.

https://reviewboard.mozilla.org/r/92132/#review93770
Attachment #8809591 - Flags: review?(ato) → review+
Comment on attachment 8809592 [details]
Bug 1316707 - Remove B2G related code from Marionette harness.

https://reviewboard.mozilla.org/r/92134/#review93772

::: testing/marionette/harness/marionette/__init__.py
(Diff revision 2)
> -    EnduranceArguments,
> -    EnduranceTestCaseMixin,
> -    HTMLReportingArguments,
> -    HTMLReportingTestResultMixin,
> -    HTMLReportingTestRunnerMixin,

Guess these were unused?

::: testing/marionette/harness/marionette/runtests.py:21
(Diff revision 2)
>  
>  
>  class MarionetteTestRunner(BaseMarionetteTestRunner):
>      def __init__(self, **kwargs):
>          BaseMarionetteTestRunner.__init__(self, **kwargs)
> -        self.test_handlers = [MarionetteTestCase, MarionetteJSTestCase]
> +        self.test_handlers = [MarionetteTestCase]

Does `testing/marionette/harness/marionette/tests/unit/tst_*.js` depend on `MarionetteJSTestCase`?
Attachment #8809592 - Flags: review?(ato) → review+
Comment on attachment 8809593 [details]
Bug 1316707 - Remove B2G related code from Marionette client.

https://reviewboard.mozilla.org/r/92136/#review93774
Attachment #8809593 - Flags: review?(ato) → review+
Comment on attachment 8809589 [details]
Bug 1316707 - Remove Marionette unit tests for B2G.

https://reviewboard.mozilla.org/r/92128/#review93768

Maja, would you mind checking that regarding Andreas feedback? Thanks.
Attachment #8809589 - Flags: review?(mjzffr)
Comment on attachment 8809592 [details]
Bug 1316707 - Remove B2G related code from Marionette harness.

https://reviewboard.mozilla.org/r/92134/#review93772

> Guess these were unused?

The HTML reporting mixin was forgotten to be removed when ported to mozlog. For endurance we haven't found any consumer beside b2g, so we can only assume no-one is using it.

> Does `testing/marionette/harness/marionette/tests/unit/tst_*.js` depend on `MarionetteJSTestCase`?

No, given that we have the `run_js_test()` method in the CommonTestCase class and with my patch those tests are still working fine. Nothing else beside B2G should have been used `MarionetteJSTestCase`.
Attachment #8809592 - Flags: review?(dburns)
Comment on attachment 8809589 [details]
Bug 1316707 - Remove Marionette unit tests for B2G.

https://reviewboard.mozilla.org/r/92128/#review93814

I'm fine with you removing those tests. Thanks for asking.
Attachment #8809589 - Flags: review?(mjzffr) → review+
Comment on attachment 8809592 [details]
Bug 1316707 - Remove B2G related code from Marionette harness.

https://reviewboard.mozilla.org/r/92134/#review93816
Attachment #8809592 - Flags: review?(dburns) → review+
Latest push only contains a rebase against the current code on mozilla-central. I'm going to push this patch series to autoland now. Thanks everyone for the reviews.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0eef3457ead7
Remove Marionette unit tests for B2G. r=ato,maja_zf
https://hg.mozilla.org/integration/autoland/rev/2a9598c33085
Remove Marionette B2G update tests. r=rstrong
https://hg.mozilla.org/integration/autoland/rev/211fbef6d74b
Remove B2G mach command for Marionette. r=ato
https://hg.mozilla.org/integration/autoland/rev/41868dc546bb
Remove B2G related code from Marionette harness. r=ato,automatedtester
https://hg.mozilla.org/integration/autoland/rev/eb8dce998e7e
Remove B2G related code from Marionette client. r=ato
Depends on: 1318478
This cleans-up our code base for Marionette a lot. It would be great to also see this uplifted to aurora. Thanks.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.