The default bug view has changed. See this FAQ.

[refactor] Test BaseMarionetteTestRunner.run_tests

RESOLVED FIXED in Firefox 50

Status

Testing
Marionette
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: vakila, Assigned: vakila)

Tracking

({ateam-marionette-harness-tests, ateam-marionette-runner})

Version 3
mozilla50
ateam-marionette-harness-tests, ateam-marionette-runner
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 months ago
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.
(Assignee)

Comment 1

10 months ago
Created attachment 8755872 [details]
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/
Attachment #8755872 - Flags: review?(mjzffr)
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. :)
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 3

10 months ago
(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.
Flags: needinfo?(mjzffr)
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.
Flags: needinfo?(mjzffr)
Keywords: ateam-marionette-runner
Summary: Add harness unit tests for resetting test statistics → [refactor] Test BaseMarionetteTestRunner.run_tests
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.
(Assignee)

Comment 6

10 months ago
Created attachment 8756378 [details]
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/
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
(Assignee)

Comment 7

10 months ago
Created attachment 8756379 [details]
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/
(Assignee)

Comment 8

10 months ago
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
(Assignee)

Comment 9

10 months ago
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/
Attachment #8755872 - Attachment description: MozReview Request: Bug 1275269 - Add tests for reset_test_stats; r?maja_zf → MozReview Request: Bug 1275269 - Refactor run_tests into smaller methods; r?maja_zf
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 10

10 months ago
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/
Attachment #8755872 - Attachment description: MozReview Request: Bug 1275269 - Refactor run_tests into smaller methods; r?maja_zf → MozReview Request: Bug 1275269 - Refactor _print_summary for run_tests; r?maja_zf
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?
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
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`.
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 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.)
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 15

10 months ago
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/
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 16

10 months ago
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/
(Assignee)

Comment 17

10 months ago
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 on attachment 8756378 [details]
Bug 1275269 - Add tests for reset_test_stats;

https://reviewboard.mozilla.org/r/55136/#review52490
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
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 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.
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 21

10 months ago
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/
Attachment #8756379 - Flags: review?(mjzffr)
Attachment #8755872 - Flags: review?(mjzffr)
Attachment #8756378 - Flags: review?(mjzffr)
(Assignee)

Comment 22

10 months ago
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/
(Assignee)

Comment 23

10 months ago
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 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.
Attachment #8756379 - Flags: review?(mjzffr)
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.
Attachment #8755872 - Flags: review?(mjzffr)
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?
Attachment #8756378 - Flags: review?(mjzffr)
(Assignee)

Comment 27

10 months ago
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?
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.
(Assignee)

Updated

10 months ago
Blocks: 1276974
(Assignee)

Comment 29

10 months ago
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
(Assignee)

Comment 30

10 months ago
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?
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.
(Assignee)

Comment 32

10 months ago
Created attachment 8758780 [details]
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/
Attachment #8756379 - Attachment description: MozReview Request: Bug 1275269 - Refactor run_tests into smaller methods; r?maja_zf → MozReview Request: Bug 1275269 - Refactor run_tests and _print_summary; r?maja_zf
Attachment #8755872 - Attachment description: MozReview Request: Bug 1275269 - Refactor _print_summary for run_tests; r?maja_zf → MozReview Request: Bug 1275269 - Add test for _initialize_test_run; r?maja_zf
Attachment #8758780 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 33

10 months ago
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/
(Assignee)

Comment 34

10 months ago
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/
(Assignee)

Comment 35

10 months ago
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/
(Assignee)

Comment 36

10 months ago
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 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.
Attachment #8756379 - Flags: review?(mjzffr)
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 on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

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

These tests look good.
Attachment #8758780 - Flags: review?(mjzffr) → review+
Attachment #8755872 - Flags: review?(mjzffr)
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 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..."
Attachment #8756378 - Flags: review?(mjzffr)
(Assignee)

Comment 42

10 months ago
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.
(Assignee)

Comment 43

10 months ago
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?
(Assignee)

Comment 44

10 months ago
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.
(Assignee)

Comment 45

10 months ago
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?
https://reviewboard.mozilla.org/r/54860/#review53730

(That's right, I mean rewrite some, not write additional.)
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.
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.)
(Assignee)

Updated

10 months ago
No longer blocks: 1276974
(Assignee)

Comment 49

9 months ago
Created attachment 8764312 [details]
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/
Attachment #8756379 - Attachment description: MozReview Request: Bug 1275269 - Refactor run_tests and _print_summary; r?maja_zf → Bug 1275269 - Refactor run_tests and _print_summary;
Attachment #8756378 - Attachment description: MozReview Request: Bug 1275269 - Add tests for reset_test_stats; r?maja_zf → Bug 1275269 - Add tests for reset_test_stats;
Attachment #8755872 - Attachment description: MozReview Request: Bug 1275269 - Add test for _initialize_test_run; r?maja_zf → Bug 1275269 - Add test for _initialize_test_run;
Attachment #8758780 - Attachment description: MozReview Request: Bug 1275269 - Add tests for _add_tests; r?maja_zf → Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;
Attachment #8764312 - Flags: review?(mjzffr)
Attachment #8756379 - Flags: review?(mjzffr)
Attachment #8756378 - Flags: review?(mjzffr)
Attachment #8755872 - Flags: review?(mjzffr)
(Assignee)

Comment 50

9 months ago
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/
(Assignee)

Comment 51

9 months ago
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/
(Assignee)

Comment 52

9 months ago
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/
(Assignee)

Comment 53

9 months ago
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 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
Attachment #8758780 - Flags: review+
Attachment #8764312 - Flags: review?(mjzffr)
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 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.
Attachment #8755872 - Flags: review?(mjzffr) → review-
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!
Attachment #8756379 - Flags: review?(mjzffr)
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.
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.
Attachment #8756378 - Flags: review?(mjzffr)
(Assignee)

Comment 60

9 months ago
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/
Attachment #8756379 - Attachment description: Bug 1275269 - Refactor run_tests and _print_summary; → Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;
Attachment #8756379 - Flags: review?(mjzffr)
Attachment #8758780 - Flags: review?(mjzffr)
(Assignee)

Comment 61

9 months ago
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/
(Assignee)

Updated

9 months ago
Attachment #8756378 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8764312 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8755872 - Attachment is obsolete: true
Comment on attachment 8756379 [details]
Bug 1275269 - Refactor & test BaseMarionetteTestRunner.run_tests;

https://reviewboard.mozilla.org/r/55138/#review57648
Attachment #8756379 - Flags: review?(mjzffr) → review+
Attachment #8758780 - Flags: review?(mjzffr) → review+
Comment on attachment 8758780 [details]
Bug 1275269 - Improve & test BaseMarionetteTestRunner._add_tests;

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

Comment 64

9 months ago
Pushed by mjzffr@gmail.com:
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
(Assignee)

Updated

9 months ago
Blocks: 1279005

Comment 65

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b60487f638fd
https://hg.mozilla.org/mozilla-central/rev/0cbfbfe2a193
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 66

9 months ago
(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
Flags: needinfo?(mjzffr)
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.
Flags: needinfo?(mjzffr)
Keywords: ateam-marionette-harness-tests
You need to log in before you can comment on or make changes to this bug.