1275269 - [refactor] Test BaseMarionetteTestRunner.run_tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Anjana Vakil (:vakila)]

Test that MarionetteTestRunner.reset_test_stats is called at the appropriate times, and that it sets the values of the Runner's test statistic properties as expected.

Comment 1

[Anjana Vakil (:vakila)]

Attachment: Bug 1275269 - Add test for _initialize_test_run;

Add harness unit tests to test that
MarionetteTestRunner.reset_test_stats is called
appropriately by run_tests, and that it sets the values
of the Runner's test statistic properties as expected.

Review commit: https://reviewboard.mozilla.org/r/54860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54860/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review51500

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:296
(Diff revision 1)
> +        with pytest.raises(OSError):
> +            runner.run_tests(tests) # should throw error after reset is called

Is the `OSError` truly related to `reset_test_stats`? I'm guessing it's to do with `tests` listing a file that doesn't exist. This looks like another piece to mock out or write differently. 

* Try passing an empty list to to `run_tests`.
* The underlying issue here is that the run_tests method is huge. As your first refactoring, you could split out some parts into separate method(s): probably a method that just does test-run initialization before running all the tests.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:305
(Diff revision 1)
> +def test_reset_test_stats_works(mach_parsed_kwargs):
> +    def assert_reset_successful(runner):
> +        stats = ['passed', 'failed', 'unexpected_successes', 'todo', 'skipped', 'failures']
> +        for s in stats:
> +            assert s in vars(runner)
> +            assert not vars(runner)[s] # should be either 0 or []

nit: please put comments in the line preceding the code they are describing

Also, favour choosing descriptive identifiers over comments. In this case, the intent of your code is quite clear on its own so I think most of the comments can be omitted. :)

Comment 3

[Anjana Vakil (:vakila)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #2)

> Is the `OSError` truly related to `reset_test_stats`? I'm guessing it's to
> do with `tests` listing a file that doesn't exist. This looks like another
> piece to mock out or write differently. 
Yep this is the issue. I can try to mock the file opening, if that's what you mean? But it would probably just raise another error a few lines later. So maybe it's best to first refactor run_tests, as you said? 
 
> * Try passing an empty list to to `run_tests`.
`[]` doesn't work because `run_tests` checks that the list is non-empty, but passing `[""]` gives the same result as passing `tests` because the file doesn't exist.

> * The underlying issue here is that the run_tests method is huge. As your
> first refactoring, you could split out some parts into separate method(s):
> probably a method that just does test-run initialization before running all
> the tests.
Agreed, this seems like the way to go. OK to open a new bug for this? I could start on it tomorrow.

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Let's work on it in this bug. You can post a series of several commits to Mozreview here - one for each set of small refactorings+tests that you develop.

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/54860/#review51586

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:300
(Diff revision 1)
> +        # reset should be called once during run_tests()
> +        with pytest.raises(OSError):
> +            runner.run_tests(tests) # should throw error after reset is called
> +        assert reset.call_count == 2
> +
> +def test_reset_test_stats_works(mach_parsed_kwargs):

nit: just `test_reset_test_stats` is enough for this test name.

Comment 6

[Anjana Vakil (:vakila)]

Attachment: Bug 1275269 - Add tests for reset_test_stats;

Add harness unit tests to test that
MarionetteTestRunner.reset_test_stats is called
appropriately by run_tests, and that it sets the values
of the Runner's test statistic properties as expected.

Review commit: https://reviewboard.mozilla.org/r/55136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55136/

Comment 7

[Anjana Vakil (:vakila)]

Attachment: Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Split the long run_tests method into several
smaller sub-methods, to make testing easier

Review commit: https://reviewboard.mozilla.org/r/55138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55138/

Comment 8

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55138/#review51778

::: testing/marionette/harness/marionette/runner/base.py:875
(Diff revision 1)
> -        self.start_time = time.time()
>  
> +    def start_marionette_if_needed(self):
>          need_external_ip = True
>          if not self.marionette:
>              self.start_marionette()

The `start_marionette()` method is a one-liner; to me it would make sense to put that line here and just get rid of that method

::: testing/marionette/harness/marionette/runner/base.py:945
(Diff revision 1)
> +    def run_tests(self, tests):
> +        self.initialize_test_run(tests)
> +        self.record_start_time()
> +
> +        need_external_ip = self.start_marionette_if_needed()
> +        self.logger.info('Initial Profile Destination is '

Can the profile path logging go in `start_marionette_if_needed()`? (I'm not sure if they conceptually go together or not)

::: testing/marionette/harness/marionette/runner/base.py:957
(Diff revision 1)
> +        for test in tests:
> +            self.add_test(test)
> +
> +        self.verify_test_names()
> +
> +        self.logger.info("running with e10s: {}".format(self.e10s))

Can the e10s logging go inside `get_version_info()`? Again, not sure about the conceptual connection

Comment 9

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/1-2/

Comment 10

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/2-3/

Comment 11

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

https://reviewboard.mozilla.org/r/55136/#review52352

Just clearning review flag - MozReview is asking me for re-review of this patch, but the code is unchanged from last time. Technical glitch?

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review52356

I like how you've broken run_tests up, overall. It's more broken up than I imagined, but let's see how that helps with testing.

::: testing/marionette/harness/marionette/runner/base.py:883
(Diff revision 1)
>              if self.capabilities['device'] == "desktop":
>                  need_external_ip = False
> -        self.logger.info('Initial Profile Destination is '
> -                         '"{}"'.format(self.marionette.profile_path))
> +        return need_external_ip
> +
> +    def record_start_time(self):
> +        self.start_time = time.time()

I don't think this would be used elsewhere, so I would just put this into `initialize_test_run` rather than its own method. 

Same sentiment for record_end_time.

::: testing/marionette/harness/marionette/runner/base.py:941
(Diff revision 1)
> +        self.initialize_test_run(tests)
> +        self.record_start_time()

General comment: the new methods you've created are not intended to be used outside of this class. You can indicate that with a leading underscore in the names like `_initialize_test_run`.

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55138/#review51778

> The `start_marionette()` method is a one-liner; to me it would make sense to put that line here and just get rid of that method

That's true. In fact, I think you can call your new `start_marionette_if_needed` method "start_marionette". (I searched -- can't find any external consumers of start_marionette, so this should be safe.)

> Can the profile path logging go in `start_marionette_if_needed()`? (I'm not sure if they conceptually go together or not)

Yes. The profile is created when starting marionette, so it make sense to keep that together.

> Can the e10s logging go inside `get_version_info()`? Again, not sure about the conceptual connection

No, it's not related to the version info.

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review52372

This is much nicer now.

::: testing/marionette/harness/marionette/runner/base.py:1020
(Diff revision 3)
> +    def close_marionette(self):
>          if self.marionette.instance:
>              self.marionette.instance.close()
>              self.marionette.instance = None
> -
>          self.marionette.cleanup()

Let's also put this in run_tests instead of separate method.

::: testing/marionette/harness/marionette/runner/base.py:1026
(Diff revision 3)
> +    def run_mixins(self, tests):
>          for run_tests in self.mixin_run_tests:
>              run_tests(tests)

Please put this in run_tests instead of new method. (I don't see the benefit of a separate method here.)

Comment 15

[Anjana Vakil (:vakila)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55136/diff/1-2/

Comment 16

[Anjana Vakil (:vakila)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55138/diff/1-2/

Comment 17

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/3-4/

Comment 18

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

https://reviewboard.mozilla.org/r/55136/#review52490

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review52494

f+ (feedback+) This looks good, carry on.

::: testing/marionette/harness/marionette/runner/base.py:780
(Diff revisions 1 - 2)
> -    def get_device_info(self):
> +    def _get_device_info(self):
>          device_info = None
>          if self.capabilities['device'] != 'desktop' and self.capabilities['browserName'] == 'B2G':
>              dm = get_dm(self.marionette)
>              device_info = dm.getInfo()
>              # Add Android version (SDK level) to mozinfo so that manifest entries
>              # can be conditional on android_version.
>              androidVersion = dm.shellCheckOutput(['getprop', 'ro.build.version.sdk'])
>              self.logger.info(
>                  "Android sdk version '%s'; will use this to filter manifests" % androidVersion)
>              mozinfo.info['android_version'] = androidVersion
>          return device_info

I think get_device_info should disappear to make sense with one of David patches to remove emulator code. Something to watch out when you rebase.

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review52498

f+ This looks good, carry on.

Comment 21

[Anjana Vakil (:vakila)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55138/diff/2-3/

Comment 22

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/4-5/

Comment 23

[Anjana Vakil (:vakila)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55136/diff/2-3/

Comment 24

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review52902

::: testing/marionette/harness/marionette/runner/base.py:816
(Diff revisions 2 - 3)
> +                                              sources=self.sources,
> +                                              dm_type=os.environ.get('DM_TRANS', 'adb') )
>  
>          self.logger.suite_start(self.tests,
>                                  version_info=version_info,
> -                                device_info=device_info)
> +                                device_info=None)

I looked up `suite_start` source code on dxr and the default for `device_info` is None, so you can just omit it.

Comment 25

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review52904

In the end, could you squash the two refactor commits into just one? I think they both count as refactoring of run_tests.

Comment 26

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

https://reviewboard.mozilla.org/r/55136/#review52914

These tests looks good. I wonder if it makes sense to add more tests related to `run_tests`. This method really pulls everything together, so testing it looks tricky (to me, anyway). Some ideas:

* Running 0 tests should not be possible I think there are actually a few redundant checks for that in BaseMarionetteTestRunner.
* Does `_verify_test_names` work?
* Does marionette get initialized to something and then get reset to None?
* If we raise a KeyboardInterrupt in the middle of a test run, does the runner re-raise it eventually after printing a summary? (We want to at least make sure that KeyboardInterrupt doesn't get ignored.)
* If we set up situation with 3 fake tests in one chunk/set and call `run_tests`, does `run_test` (no plural) in turn get called 3 times? Maybe this idea is too broad - many things would have to be mocked out. Worth investigating?

Comment 27

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55136/#review52914

Agreed, more tests for the various `run_tests` components sounds like a great idea. 

> Running 0 tests should not be possible

`_initialize_test_run` checks that `tests` is non-empty. I'm adding a test for that method, which includes a check that an empty list of tests raises `AssertionError`.

> Does `_verify_test_names` work?

Looking at `_verify_test_names`, I'm wondering why the name verification is done after tests are added, instead of in `add_test` before we even append the test to `self.tests`?
On that subject, I'd also like to add a test for `add_test` (ha), to make sure that e.g. `.ini` files and directories are handled appropriately. How does that sound?

Comment 28

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55136/#review52914

Yeah, that's a good point: you can check `filepath` right before `self.tests.append()` and raise an Exception if necessary -- no need for `_verify_test_names`. 

Please file a bug for testing `add_test`  -- it might get more involved and we should prioritize it separately.

Comment 29

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55136/#review52914

The only advantage I can see to having a separate `_verify_test_names` is that it doesn't throw an Exception for each individual invalid name, but rather gives you a list of all of them. If we do it in `add_test`, would it be annoying to get an Exception for one bad filename, fix it, and then get another Exception for the next bad file? 

> Please file a bug for testing add_test

Will do

Comment 30

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55136/#review52914

I just noticed Bug 1097586, specifically [this comment](https://bugzilla.mozilla.org/show_bug.cgi?id=1097586#c2), which suggests adding a method like:

    def add_tests(self, tests):
       for test in tests:
          self.add_test(test)

which can then be called in `run_tests`. Then, to preserve the only-one-Exception-for-multiple-bad-filenames idea, we could put the functionality of `_verify_test_names` inside `add_tests` (plural). Thoughts?

Comment 31

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55136/#review52914

Having a list of all invalid test names in one error message is definitely better than one test at a time, but not crucial. I like your idea of `add_tests` + one Exeception.

Comment 32

[Anjana Vakil (:vakila)]

Attachment: Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

Add tests for BaseMarionetteTestRunner._add_tests:
Test that _add_tests populates self.tests with correct tests;
Test that invalid test names cause _add_tests to
throw Exception and report invalid names as expected.

Review commit: https://reviewboard.mozilla.org/r/56938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56938/

Comment 33

[Anjana Vakil (:vakila)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55138/diff/3-4/

Comment 34

[Anjana Vakil (:vakila)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55136/diff/3-4/

Comment 35

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/5-6/

Comment 36

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55138/#review53640

::: testing/marionette/harness/marionette/runner/base.py:787
(Diff revision 4)
>              self.add_test(test)
>  
> -        # ensure we have only tests files with names starting with 'test_'
>          invalid_tests = \
>              [t['filepath'] for t in self.tests
>               if not os.path.basename(t['filepath']).startswith('test_')]

The `add_test` (singular) method seems to indicate that test names need not only to start with `"test_"`, but also to end with `".py"` or `".js"`. 

Should we modify this (and the corresponding test) to check the extension as well?

Comment 37

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review53692

r+wc (review granted, with comments to address)

::: testing/marionette/harness/marionette/runner/base.py:803
(Diff revision 4)
> +        self._initialize_test_run(tests)
> +
> +        need_external_ip = self._start_marionette()
> +        self._set_baseurl(need_external_ip)
> +
> +        self._add_tests(tests)

Did you say earlier that you wanted to call `_add_tests` before `_start_marionette` and incorporate `_initialize_test_run` into it? Not sure I remember right, but go head with that if it makes testing easier.

Comment 38

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55138/#review53640

> The `add_test` (singular) method seems to indicate that test names need not only to start with `"test_"`, but also to end with `".py"` or `".js"`. 
> 
> Should we modify this (and the corresponding test) to check the extension as well?

Ah, good catch. Yes, check extensions. Right now add_test is only checking extensions in the "directory" case, but it actually looks like individual files and files listed in manifests were intended to be checked for that as well: there an unused file_ext variable (https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#932). Must have been overlooked.

When I run something like `./mach marionette-test --address=localhost:2828 test_bad_file.txt`, the runner reports that 0 tests were run rather than throwing an error about the bad extension.

Comment 39

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

https://reviewboard.mozilla.org/r/56938/#review53740

These tests look good.

Comment 40

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review53730

A general comment about testing these helper methods like `_initialize_test_run`, `reset_test_stats`, `verify_test_names`, etc: on the one hand we want to verify that they work correctly, especially if they have input parameters, on the other hand we want to verify that `run_tests` does all the things it's supposed to, since that's the method that consumers are most likely to interact with. So I'd like to see some tests that exercise the functionality of the helper methods by calling `run_tests`. For example, a test that sets up a runner with certain parameters, mocks some things\* out, calls `run_tests` and checks whether an error was raised if we passed in an empty list of tests (rather than testing that by calling `_initialize_test_run` directly). Similarly, we can have a test that calls `run_tests` and verifies that `reset_test_stats` got called once (rather than checking that `_initialize_test_run` calls `reset_test_stats` once. Does that make sense?

I don't think that much needs to be mocked out to test `run_tests`, but I could be wrong. You can make a new "runner" fixture that is based on `runner` and `mock_marionette` fixtures (and has `runner.driverclass` mocked out). I think `mock_marionette` will need to use `MagicMock` instead of `Mock`. And then you can mock `add_tests` to return something that your test can work with; same with `run_test_sets`.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:323
(Diff revision 6)
>      assert_reset_successful(runner)
>  
>  
> +def test_initialize_test_run(mach_parsed_kwargs):
> +    tests = mach_parsed_kwargs.pop('tests')
> +    runner = MarionetteTestRunner(**mach_parsed_kwargs)

You can omit lines like this by using the `runner` fixture instead of the `mach_parsed_kwargs` fixture. See https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py#125

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:326
(Diff revision 6)
> +def test_initialize_test_run(mach_parsed_kwargs):
> +    tests = mach_parsed_kwargs.pop('tests')
> +    runner = MarionetteTestRunner(**mach_parsed_kwargs)
> +    runner._initialize_test_run(tests)
> +    assert runner.start_time and type(runner.start_time) is float
> +    # Does it make sense to move some of the rest_test_stats testing here?

The reset_test_stats testing is good as is, no need to move it.

Comment 41

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

https://reviewboard.mozilla.org/r/55136/#review53742

These tests are good as well, but I want to exercise `run_tests` rather than only its helper methods.

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:302
(Diff revision 4)
> +def test_reset_test_stats_is_called(mach_parsed_kwargs):
> +    tests = mach_parsed_kwargs.pop('tests')
> +    with patch('marionette.runtests.MarionetteTestRunner.reset_test_stats', spec=True) as reset:
> +        runner = MarionetteTestRunner(**mach_parsed_kwargs)
> +        assert reset.call_count == 1
> +        runner._initialize_test_run(tests)

See my other comment - "A general comment about testing..."

Comment 42

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55138/#review53692

> Did you say earlier that you wanted to call `_add_tests` before `_start_marionette` and incorporate `_initialize_test_run` into it? Not sure I remember right, but go head with that if it makes testing easier.

You remember correctly. I tried it, but it turns out that order doesn't work because in the case where a manifest is used, `add_test` needs the `device` property, which needs the `component` property, which needs `self.marionette`. Took me a minute to figure that one out, but it seems like that's why it was in this order in the first place.

Comment 43

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55138/#review53640

> Ah, good catch. Yes, check extensions. Right now add_test is only checking extensions in the "directory" case, but it actually looks like individual files and files listed in manifests were intended to be checked for that as well: there an unused file_ext variable (https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#932). Must have been overlooked.
> 
> When I run something like `./mach marionette-test --address=localhost:2828 test_bad_file.txt`, the runner reports that 0 tests were run rather than throwing an error about the bad extension.

Indeed. For now I'm just going to check extensions in `_add_tests` (plural) after they've been added to `self.tests`, for the same reason as before (reporting multiple bad filenames as a single error), but this is giving me some ideas for refactoring `add_test` (singular) for Bug 1276974. I think ultimately it would be good to have the filename-checking and `self.tests`-populating functionality be separate and self-contained (and ideally in that order), instead of e.g. checking names in the "directory" case in `add_test` but other cases after the fact in `_add_tests`; what do you think?

Comment 44

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/54860/#review53730

Sure, I see the logic there. I'll rewrite most of these tests to call `run_tests` directly rather than one of the submethods. (Or did you mean we should have *additional* tests that call `run_tests`?)

I probably need to read up on fixtures/mocking, so this might take me a little while but I'll figure it out.

Comment 45

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/55138/#review53640

> Indeed. For now I'm just going to check extensions in `_add_tests` (plural) after they've been added to `self.tests`, for the same reason as before (reporting multiple bad filenames as a single error), but this is giving me some ideas for refactoring `add_test` (singular) for Bug 1276974. I think ultimately it would be good to have the filename-checking and `self.tests`-populating functionality be separate and self-contained (and ideally in that order), instead of e.g. checking names in the "directory" case in `add_test` but other cases after the fact in `_add_tests`; what do you think?

Hm, I was just looking at http://mozbase.readthedocs.io/en/latest/manifestparser.html and some of the tests mentioned in examples there have names like 'testSomeThing.js'. Right now we'd be excluding those because they're missing the `_` after `test`. Is that the behavior we want?

Comment 46

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/54860/#review53730

(That's right, I mean rewrite some, not write additional.)

Comment 47

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55138/#review53640

> Hm, I was just looking at http://mozbase.readthedocs.io/en/latest/manifestparser.html and some of the tests mentioned in examples there have names like 'testSomeThing.js'. Right now we'd be excluding those because they're missing the `_` after `test`. Is that the behavior we want?

We definitely want that for Python files. You could have different cases for js and py files. re library might be a cleaner way to do it.

Comment 48

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/55138/#review53640

> We definitely want that for Python files. You could have different cases for js and py files. re library might be a cleaner way to do it.

(FYI, I replied to your question about add_test in Bug 1276974.)

Comment 49

[Anjana Vakil (:vakila)]

Attachment: Bug 1275269 - Create runner_mock fixture for run_tests testing;

Create new pytest fixture `runner_mock` to get a
runner instance with some properties (e.g. `self.marionette`)
mocked out, for use in some new tests for `run_tests`.

Review commit: https://reviewboard.mozilla.org/r/60272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60272/

Comment 50

[Anjana Vakil (:vakila)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55138/diff/4-5/

Comment 51

[Anjana Vakil (:vakila)]

Comment on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55136/diff/4-5/

Comment 52

[Anjana Vakil (:vakila)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54860/diff/6-7/

Comment 53

[Anjana Vakil (:vakila)]

Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56938/diff/1-2/

Comment 54

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

https://reviewboard.mozilla.org/r/56938/#review57164

::: testing/marionette/harness/marionette/runner/base.py:792
(Diff revision 2)
>          for test in tests:
>              self.add_test(test)
>  
> -        invalid_tests = \
> -            [t['filepath'] for t in self.tests
> -             if not os.path.basename(t['filepath']).startswith('test_')]
> +        def is_valid(test):
> +            filename = os.path.basename(test['filepath'])
> +            return re.match("^test(((_.+?)+?\.((py)|(js)))|(([A-Z].*?)+?\.js))$", filename)

Since you'll be calling re.match in a loop, it's more efficient to re.compile the regex first (outside of is_valid) so that it can be reused in the loop.

::: testing/marionette/harness/marionette/runner/base.py:796
(Diff revision 2)
> -            [t['filepath'] for t in self.tests
> -             if not os.path.basename(t['filepath']).startswith('test_')]
> +            filename = os.path.basename(test['filepath'])
> +            return re.match("^test(((_.+?)+?\.((py)|(js)))|(([A-Z].*?)+?\.js))$", filename)
> +        invalid_tests = [t['filepath'] for t in self.tests if not is_valid(t)]
>          if invalid_tests:
> -            raise Exception("Tests file names must start with 'test_'."
> +            raise Exception("Test file names must be of the form "
> +                            "'test_this_thing.py', 'test_this_thing.js', or 'testThisThing.js'."

Nit: use example with only one underscore since all we check for is test_ and .py

Comment 55

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8764312 [details]
Bug 1275269 - Create runner_mock fixture for run_tests testing;

https://reviewboard.mozilla.org/r/60272/#review57166

Clearing review flag because these changes appear in two consecutive commits. (See review in "Add test for _initialize_test_run" as well.)

Comment 56

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8755872 [details]
Bug 1275269 - Add test for _initialize_test_run;

https://reviewboard.mozilla.org/r/54860/#review57168

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:31
(Diff revisions 6 - 7)
>  
>  
>  @pytest.fixture()
>  def mock_marionette(request):
>      """ Mock marionette instance """
> -    import marionette_driver
> +    marionette = Mock(spec=Marionette)

MagicMock probably handy here (especially if this fixture will be used by `runner_mock`.

Also nit: choose an consistent naming scheme (mock_blah or blah_mock)

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:140
(Diff revisions 6 - 7)
> +    MarionetteTestRunner instance with mocked-out
> +    self.marionette and other properties.
> +    """
> +    mach_parsed_kwargs['binary'] = None
> +    runner = MarionetteTestRunner(**mach_parsed_kwargs)
> +    runner.driverclass = MagicMock(spec=Marionette)

Would it make sense to have runner_mock to depend on mock_marionette? (i.e. `def runner_mock(mock_marionette, mach_parsed_kwargs)` and `runner.driverclass = mock_marionette`)

::: testing/marionette/harness/marionette/tests/harness_unit/test_marionette_runner.py:386
(Diff revisions 6 - 7)
>          for s in stats:
> -            assert s in vars(runner)
> -            assert not vars(runner)[s]
> -    runner = MarionetteTestRunner(**mach_parsed_kwargs)
> +            if not ((s in vars(runner)) and (not vars(runner)[s])):
> +                success = False
> +        return success

I think this can be replaced with a call to the `all` Python built-in fn.

Comment 57

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review57170

::: testing/marionette/harness/marionette/runner/base.py:759
(Diff revision 5)
>  
> -    def run_tests(self, tests):
> +    def _initialize_test_run(self, tests):
>          assert len(tests) > 0
>          assert len(self.test_handlers) > 0
>          self.reset_test_stats()
>          self.start_time = time.time()

I think it will be clearer to define both start_time and end_time directly in run_tests (rather than start_time in `_initialize_test_run`). Also, start_time and end_time aren't used anywhere else, so we can make them local variables instead of instead variables. Less shared state, yay!

Comment 58

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Almost there! :) I'm pretty sure I'm running out of suggestions, heh. 

Re commits - first 4 can be squashed down to one, + keep the "Improve & test BaseMarionetteTestRunner._add_tests" commit.

Comment 59

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

And also sorry if some of my comments seem out of order or strangely placed:after your most recent rebase, MozReview was presenting changes in a way that was hard to me to follow. Not your fault.

Comment 60

[Anjana Vakil (:vakila)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55138/diff/5-6/

Comment 61

[Anjana Vakil (:vakila)]

Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56938/diff/2-3/

Comment 62

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review57648

Comment 63

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

https://reviewboard.mozilla.org/r/56938/#review57650

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60487f638fd
Refactor & test BaseMarionetteTestRunner.run_tests; r=maja_zf
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbfbfe2a193
Improve & test BaseMarionetteTestRunner._add_tests; r=maja_zf

Comment 65

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b60487f638fd
https://hg.mozilla.org/mozilla-central/rev/0cbfbfe2a193

Comment 66

[Anjana Vakil (:vakila)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #47)
> https://reviewboard.mozilla.org/r/55138/#review53640
> 
> > Hm, I was just looking at http://mozbase.readthedocs.io/en/latest/manifestparser.html and some of the tests mentioned in examples there have names like 'testSomeThing.js'. Right now we'd be excluding those because they're missing the `_` after `test`. Is that the behavior we want?
> 
> We definitely want that for Python files. You could have different cases for
> js and py files. re library might be a cleaner way to do it.

I just noticed that the MDN page about Marionette JS tests [1] says in no uncertain terms:

> Marionette supports WebAPI tests written in JavaScript.  These tests must have a filename that begins with `test_` and must have the extension `.js`.

Should I revert the change I made to allow `testSomething.js` names? Or update the MDN page? Or just let it be, since `test_something.js` will still work fine?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Marionette_JavaScript_Tests

Comment 67

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Thanks for double-checking that. 

Please leave everything as is (allowing both testSomething.js and test_something.js). As I understand it, WebAPI tests were just for B2G, which is not active anymore.