1313593 - Marionette unit tests should not be located at /testing/marionette/harness/marionette/tests/unit/

Status: RESOLVED INCOMPLETE

(Remote Protocol :: Marionette, defect)

Description

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

Right now the driver unit test are located under the harness but actually should be in /testing/marionette/client/tests. 

Before we start on this bug we should wait for the big refactoring on bug 1259055 to have landed.

Comment 1

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

Well, those are actually client unit tests.

Comment 2

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

So the client unit tests actually make use of the MarionetteTestCase class which is defined in the harness package. And the latter depends on the former. That means that we actually cannot move the tests over to the client folder without adding a circular dependency.

I wonder why this decision has been made and the MarionetteTestCase class been put into the harness instead of the client package. It doesn't allow us to really create package internal tests.

David and/or Andreas, can you remember something?

Comment 3

[Andreas Tolfsen ❲:ato❳]

In my honest opinion it is strange for non-client tests to live inside _either_ the harness- and the client packages.  Presumably tests for such features as UI, accessibility, and DOM caret are _consumers_ of the Marionette harness and not integral parts of it.

To move all the tests currently in testing/marionette/harness/marionette/tests/unit to testing/marionette/client/test presupposes that they are all client API tests, which isn’t true.  They are currently a mix of server and client tests, and in effect serve as the official test suite for the Marionette server (JS code).  I would assume that integration tests that target the server should live somewhere in testing/marionette and be consumers of both the harness and the client.

But it is also true that these tests _should not_ live under testing/marionette/harness.  I would expect the tests there to be only the Mn-h tests specifically testing the features of the harness.  Correspondingly, I would expect tests in testing/marionette/client to test (mostly) client features.

The historical reason for the current structure is “legacy”, but I feel that if we want to address this situation, we need to look deeper at the relationship between the harness and the client and how to make them less interdependent.

Conceptually, MarionetteTestCase does belong in the test harness as it isn’t related to implementing a Marionette client in any way.  The fact that the harness depends on the client seems natural, but I guess it’s more questionable that the reverse is also the case: is it correct that the client should depend on the harness?  I think it is fine for client integration tests to do so, but we don’t want this to be a published dependency in requirements.txt.

Comment 4

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

(In reply to Andreas Tolfsen ‹:ato› from comment #3)
> To move all the tests currently in
> testing/marionette/harness/marionette/tests/unit to
> testing/marionette/client/test presupposes that they are all client API
> tests, which isn’t true.  They are currently a mix of server and client
> tests, and in effect serve as the official test suite for the Marionette
> server (JS code).  I would assume that integration tests that target the
> server should live somewhere in testing/marionette and be consumers of both
> the harness and the client.

Ok, so that means we would have to check the tests and divide them up at some time. Especially those tests which are for the server only, and were covered as unittest because there was no unit test suite for server available at the time of writing, might have to be rewritten as pure xpcshell tests, right? 

> But it is also true that these tests _should not_ live under
> testing/marionette/harness.  I would expect the tests there to be only the
> Mn-h tests specifically testing the features of the harness. 
> Correspondingly, I would expect tests in testing/marionette/client to test
> (mostly) client features.
>
> The historical reason for the current structure is “legacy”, but I feel that
> if we want to address this situation, we need to look deeper at the
> relationship between the harness and the client and how to make them less
> interdependent.
> 

As of right now it would mean that those tests would have to use the basic unittest.TestCase class or pytest similar to the harness unit tests. But given that this is much work work to do - and maybe not worth our time at all right now - we might want to do that at a later time.

So what about the following for now: Lets create a new folder like testing/marionette/test which can contain two sub folders. One for the existing real xpcshell Marionette server tests, so that those are not laying next to the code modules in the same folder. And then another subfolder, lets call it integration(?) for now, which will contain a mix of client and server tests.

Comment 5

[David Burns :automatedtester]

To be honest, this work is far at the bottom of the priorities at the moment and can be put on hold

Comment 6

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Ok, so that means we would have to check the tests and divide them up at
> some time. Especially those tests which are for the server only, and were
> covered as unittest because there was no unit test suite for server
> available at the time of writing, might have to be rewritten as pure
> xpcshell tests, right? 

Not necessarily rewritten as xpcshell tests.  I think the long-term plan here should be to port tests to WPT (testing/web-platform/tests/webdriver) and make the Wd Tier-1 and deprecate the server-specific tests.

Those we cannot upstream to WPT we could probably leave as part of the client tests.  We could consider them client integration tests.  I think the harder problem is figuring out the dependency relationship between the client and the harness.  Is it possible (in Python) to make the client tests depend on the harness, but not the client package itself?

> As of right now it would mean that those tests would have to use the basic
> unittest.TestCase class or pytest similar to the harness unit tests. But
> given that this is much work work to do - and maybe not worth our time at
> all right now - we might want to do that at a later time.

I agree.  If we really want to get deep into this, we should consider writing a pytest harness IMO.

(In reply to David Burns :automatedtester from comment #5)
> To be honest, this work is far at the bottom of the priorities at the moment
> and can be put on hold

I concur.  The reason I didn’t do this when renaming the testing/marionette/{client,harness} directories is that it would be a lot of work.  I do think the current situation is bad, but it works.

Looking into the future, I feel quite strongly that we should do a review on the Marionette harness as something we offer to internal consumers and what we can do to improve it.  We want to ensure adoption of Marionette-backed integration tests are easy to write.

Comment 7

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

Ok, so we will live with the current structure for now, and I will put my focus on other important bugs.

Comment 8

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

Closing as incomplete for now.