1272109 - Lots of Marionette unittests not executed on desktop because "@skip_if_b2g" is set on test class level

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

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

As I have seen today while working on bug 1271758, lots of our unit tests aren't getting run on desktop because the skip decorator for b2g is set on class level. Due to that the whole test class is skipped on every platform!

Here the instances:

http://mxr.mozilla.org/mozilla-central/search?string=%40skip_if_b2g&find=testing/marionette/harness/marionette

If we have more decorators and those are also attached on the class, it would be the same problem. 

Due to that we miss a lot of test coverage!

Comment 1

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

Quick scan showed me the following entries:

tests/unit/test_elementsize.py
tests/unit/test_execute_script.py
tests/unit/test_window_title.py

Comment 2

[David Burns :automatedtester]

WIth Skip_if_b2g

Ran 664 tests
Expected results: 627
Unexpected results: 5 (ERROR: 2, FAIL: 3)
Skipped: 32

without

Ran 695 tests
Expected results: 653
Unexpected results: 10 (ERROR: 5, FAIL: 5)
Skipped: 32

Looks like we were missing 31 tests

Comment 3

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

Please notice the skip count of both! It's in each case 32. That means with the skip decorator we do not count those tests as skipped. We simply loose them.

Comment 4

[David Burns :automatedtester]

Looks like I broke this when I landed bug 965308

Comment 5

[David Burns :automatedtester]

Attachment: MozReview Request: Bug 1272109: Remove skip_if_b2g decorator for tests

It looks like when I landed bug 965308 it stopped looking at any tests
that had @skip_if_b2g decorator. Since we no longer need to support b2g
we should just remove the decorator.

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

FWIW it is possible to test the platformName capability if it matches "B2G" instead of testing for the presence of the b2g capability.

I notice that testing/marionette/harness/marionette/marionette_test.py has quite a few references to the latter, which was removed in bug 965308.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Also I’m not sure if removing support for disabling tests on B2G is a good idea at this point.  Are we confident they are not being run or will be activated again in the future?

Comment 8

[David Burns :automatedtester]

(In reply to Andreas Tolfsen ‹:ato› from comment #7)
> Also I’m not sure if removing support for disabling tests on B2G is a good
> idea at this point.  Are we confident they are not being run or will be
> activated again in the future?

The python tests have been disabled as, just before the announcement in mozlando, they were converting them to marionettejs. I am happy to remove this support.

Also... https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-tier=3 shows that B2G hasnt been building for a while so even less concerned now...

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8751698 [details]
MozReview Request: Bug 1272109: Remove skip_if_b2g decorator for tests

https://reviewboard.mozilla.org/r/52163/#review49099

That makes sense then.  Can you make sure you do a try run as well?

Comment 10

[David Burns :automatedtester]

(In reply to Andreas Tolfsen ‹:ato› from comment #9)
> Comment on attachment 8751698 [details]
> MozReview Request: Bug 1272109: Remove skip_if_b2g decorator for tests
> 
> https://reviewboard.mozilla.org/r/52163/#review49099
> 
> That makes sense then.  Can you make sure you do a try run as well?

Most definitely, there are executeScript failures that I would like to see if they are gone with bug 1271758 and rebase on top of that before I do a try run.

Comment 11

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

My patch landed, but afair the failures haven't been fixed with it given that my patch has very specific conditions. So all the failures should still be present.

Comment 12

[Andreas Tolfsen ❲:ato❳]

I’m looking at the fallout in the test_execute_script.py tests.

Comment 13

[David Burns :automatedtester]

Comment on attachment 8751698 [details]
MozReview Request: Bug 1272109: Remove skip_if_b2g decorator for tests

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/613c6e5d56a2

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failure in test_execute_script.py.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f23a7dd307
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=613c6e5d56a216823cdc11d85f4408c778160e2d
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=28013326&repo=mozilla-inbound

04:14:57     INFO -  TEST-START | test_execute_script.py TestExecuteChrome.test_lasting_side_effects
04:14:57     INFO -  Failed to gather test failure debug.
04:14:57     INFO -  Traceback (most recent call last):
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette\runner\base.py", line 614, in gather_debug
04:14:57     INFO -      rv['source'] = marionette.page_source
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 1319, in page_source
04:14:57     INFO -      return self._send_message("getPageSource", key="value")
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette_driver\decorators.py", line 36, in _
04:14:57     INFO -      return func(*args, **kwargs)
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 757, in _send_message
04:14:57     INFO -      self._handle_error(err)
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette_driver\marionette.py", line 818, in _handle_error
04:14:57     INFO -      raise errors.lookup(error)(message, stacktrace=stacktrace)
04:14:57     INFO -  NoSuchFrameException: NoSuchFrameException: No such content frame; perhaps the listener was not registered?
04:14:57    ERROR -  TEST-UNEXPECTED-FAIL | test_execute_script.py TestExecuteChrome.test_lasting_side_effects | AssertionError: False is not true
04:14:57     INFO -  Traceback (most recent call last):
04:14:57     INFO -    File "C:\slave\test\build\venv\lib\site-packages\marionette\marionette_test.py", line 344, in run
04:14:57     INFO -      testMethod()
04:14:57     INFO -    File "C:\slave\test\build\tests\marionette\tests\testing\marionette\harness\marionette\tests\unit\test_execute_script.py", line 233, in test_lasting_side_effects
04:14:57     INFO -      send("return typeof window.wrappedJSObject == 'undefined'"))

Comment 16

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

The patch should not have landed before bug 1272642 got fixed. Ideally those tests should have been fixed immediately to reduce the dependency.

Comment 17

[David Burns :automatedtester]

The dependent bug hasnt landed and I thought it did... which caused the backout.

Comment 18

[David Burns :automatedtester]

https://hg.mozilla.org/integration/mozilla-inbound/rev/112f894e71ff9ea4f2d4629560b8bcdee3185485
Bug 1272109: Remove skip_if_b2g decorator for tests r=ato

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/112f894e71ff

Comment 20

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

Moving bugs for Marionette client due to component changes.