All users were logged out of Bugzilla on October 13th, 2018

Revise custom skip decorators of Marionette harness

RESOLVED FIXED in Firefox 52

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Version 3
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
The following issues we currently have:

* For our custom skip decorators Marionette creates screenshots in the HTML report. See bug 1275243.

* There is no way to add additional information via a message parameter
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Attachment #8820779 - Flags: review?(james)
Attachment #8820780 - Flags: review?(mjzffr)
Attachment #8820781 - Flags: review?(mjzffr)
Attachment #8820782 - Flags: review?(mjzffr)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8820780 [details]
Bug 1323770 - Marionette should not take screenshots for skipped tests.

https://reviewboard.mozilla.org/r/100214/#review100874
Attachment #8820780 - Flags: review?(mjzffr) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8820782 [details]
Bug 1323770 - Fix inappropriatelly skipped/disabled tests.

https://reviewboard.mozilla.org/r/100218/#review100876
Attachment #8820782 - Flags: review?(mjzffr) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8820781 [details]
Bug 1323770 - Fix skip decorators for unit tests.

https://reviewboard.mozilla.org/r/100216/#review101146

Yay for better decorators, thank you.

::: testing/marionette/harness/marionette_harness/marionette_test/__init__.py:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  __version__ = '3.1.0'
>  

General comment about the commit message: please add more detail, as we discussed. Some ideas: In what way are you fixing the decorators? Why? What's broken about them? What's the old behaviour and the new behaviour? Thanks!

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:53
(Diff revision 1)
>  
> -def run_if_e10s(target):
> +def run_if_e10s(reason):
>      """Decorator which runs a test if e10s mode is active."""
> -    def wrapper(self, *args, **kwargs):
> +    def decorator(test_item):
> +        if not isinstance(test_item, types.FunctionType):
> +            raise Exception('run_if_e10s not supported for classes')

Nice addition, that will save us some pain in the future. :)

Nit to make the message more accurate here and in all the other decorators: s/not supported for classes/only supported for functions/

To avoid repetition, you might want to just define a single message to reuse everwhere, like "Decorator only supported for functions."

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:65
(Diff revision 1)
> -              return Services.appinfo.browserTabsRemoteAutostart;
> +                      return Services.appinfo.browserTabsRemoteAutostart;
> -            } catch (e) {
> +                    } catch (e) {
> -              return false;
> +                      return false;
> -            }""")
> -
> -        if not multi_process_browser:
> +                    }
> +                """):
> +                    raise SkipTest(reason)

I think `if not multi_process_browser:` is more readable here and in `skip_if_e10s`

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:66
(Diff revision 1)
> -            } catch (e) {
> +                    } catch (e) {
> -              return false;
> +                      return false;
> -            }""")
> -
> -        if not multi_process_browser:
> -            raise SkipTest('skipping due to e10s is disabled')
> +                    }
> +                """):
> +                    raise SkipTest(reason)
> +            return test_item(self)

Safer to do `test_item(self, *args, **kwargs)` everywhere

Let's not strip off function arguments unless it's actually necessary.

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:144
(Diff revision 1)
>      """Decorator which skips a test based on the value of a browser preference.
>  
>      :param pref: the preference name
>      :param predicate: a function that should return false to skip the test.
>                        The function takes one parameter, the preference value.
>                        Defaults to the python built-in bool function.

Either add back `predicate=bool` or remove this line.

::: testing/marionette/harness/marionette_harness/marionette_test/decorators.py:160
(Diff revision 1)
>                pass  # test implementation here
> +
>      """
> -    def wrapper(target):
> -        @functools.wraps(target)
> -        def wrapped(self, *args, **kwargs):
> +    def decorator(test_item):
> +        if not isinstance(test_item, types.FunctionType):
> +            raise Exception('skip_unless_protocol not supported for classes')

s/skip_unless_protocol/skip_unless_browser_pref/

::: testing/marionette/harness/marionette_harness/tests/unit/test_screen_orientation.py:83
(Diff revision 1)
> -    @skip_if_desktop
> +    @skip_if_desktop("Not supported in Firefox")
>      def test_set_null_orientation(self):
>          with self.assertRaisesRegexp(errors.MarionetteException, unknown_orientation.format("null")):
>              self.marionette.set_orientation(None)
>  
> -    @skip_if_mobile
> +    @skip_if_mobile("Specifc test for Firefox")

"Specific"
Attachment #8820781 - Flags: review?(mjzffr) → review+
(Assignee)

Updated

2 years ago
Blocks: 1326047
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8820779 - Flags: review?(mjzffr)
Attachment #8820779 - Flags: review?(james)
Attachment #8820779 - Flags: review?(ahalberstadt)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8820779 [details]
Bug 1323770 - Moztest should forward correct test result.

https://reviewboard.mozilla.org/r/100212/#review101722

Lgtm. Also looks like marionette is the only thing that uses the StructuredTestResult class, so should be safe to land.
Attachment #8820779 - Flags: review?(ahalberstadt) → review+
(Assignee)

Updated

2 years ago
Attachment #8820779 - Flags: review?(mjzffr)
(Assignee)

Updated

2 years ago
Keywords: leave-open
Whiteboard: [needs pypi release]
(Assignee)

Updated

2 years ago
status-firefox52: --- → affected
status-firefox53: --- → affected
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8820779 [details]
Bug 1323770 - Moztest should forward correct test result.

https://reviewboard.mozilla.org/r/100210/#review101734

I had to rebase this patch series and fix the merge conflicts. This is an updated version which will be ready for landing.

Comment 18

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71e82343afb9
Moztest should forward correct test result. r=ahal
https://hg.mozilla.org/integration/autoland/rev/d0e5cb3af786
Marionette should not take screenshots for skipped tests. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/f1fbf0853e4f
Fix skip decorators for unit tests. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/bb8ccabdbe27
Fix inappropriatelly skipped/disabled tests. r=maja_zf
(Assignee)

Comment 20

2 years ago
The problem here was that my patch on bug 1243415 landed on autoland a couple of hours before, and that it added some skip decorators. My last rebase against mozilla-central for this patch series, didn't see those changes and as such was working fine on try. But landed on autoland too the failures were visible.

Given that all patches have been merged to mozilla-central now, I can go ahead and do another rebase before landing the series again.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5747f6e116d7
Moztest should forward correct test result. r=ahal
https://hg.mozilla.org/integration/autoland/rev/242e9ccd7219
Marionette should not take screenshots for skipped tests. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/bd34008c64a4
Fix skip decorators for unit tests. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/b95245f46325
Fix inappropriatelly skipped/disabled tests. r=maja_zf
(Assignee)

Comment 27

2 years ago
moztest 0.8 has been releated to PyPI:

creating build/bdist.macosx-10.11-x86_64/wheel/moztest-0.8.dist-info/WHEEL
Uploading distributions to https://pypi.python.org/pypi
Uploading moztest-0.8-py2-none-any.whl
[================================] 14232/14232 - 00:00:04
Uploading moztest-0.8.tar.gz
[================================] 10527/10527 - 00:00:02
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [needs pypi release]
Target Milestone: --- → mozilla53
(Assignee)

Comment 28

2 years ago
A test-only change which is wanted on aurora for the next ESR release. When uplifting please make sure to uplift the patch on bug 1243415 first. Thanks.
Blocks: 1325079
Whiteboard: [checkin-needed-aurora][uplift patch on bug 1243415 first]
You need to log in before you can comment on or make changes to this bug.