Closed Bug 1323770 Opened 8 years ago Closed 7 years ago

Revise custom skip decorators of Marionette harness

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(4 files)

Bug 1323770 - Moztest should forward correct test result.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1323770 - Marionette should not take screenshots for skipped tests.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1323770 - Fix skip decorators for unit tests.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1323770 - Fix inappropriatelly skipped/disabled tests.
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
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
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #8820779 - Flags: review?(james)
Attachment #8820780 - Flags: review?(mjzffr)
Attachment #8820781 - Flags: review?(mjzffr)
Attachment #8820782 - Flags: review?(mjzffr)
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 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 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+
Blocks: 1326047
Attachment #8820779 - Flags: review?(mjzffr)
Attachment #8820779 - Flags: review?(james)
Attachment #8820779 - Flags: review?(ahalberstadt)
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+
Attachment #8820779 - Flags: review?(mjzffr)
Keywords: leave-open
Whiteboard: [needs pypi release]
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.
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
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)
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
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
Closed: 7 years ago
status-firefox53: affected → fixed
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [needs pypi release]
Target Milestone: --- → mozilla53
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]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: