807095 - JS assertion failures in a Python test case do not cause the test to be marked as failed

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

If a Python test case uses execute_(async_)script to execute a block of JS that includes assertion failures, the failures get written to the log but do not cause the test to fail.

Case in point:

http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_voicemail_statuschanged.py

On a recent run on TBPL, this failed:

13:24:08     INFO - I/Gecko   (  385): MARIONETTE TEST RESULT:TEST-UNEXPECTED-FAIL | test_voicemail_statuschanged.py:TestVoicemailStatusChanged.testStatusChanged | undefined - false was false, expected true

...but the test was marked as passed.

Comment 1

[Jonathan Griffin (:jgriffin)]

Full log:  https://tbpl.mozilla.org/php/getParsedLog.php?id=16595876&tree=Cedar#error10

Comment 2

[mathieu bultel:mbu]

Ok,

I don't have access to the link above, but i reproduce the bug.
I'll focus on it this week-end.

Comment 3

[mathieu bultel:mbu]

Yep

Sorry for the delay, i have found a way to fix it, but i have one worry/question :

- execute_script(), return None each time, cause of non-async method (?).. so in the case of execute_script, if we did not have the status of the test, we can't thrown a failure in the test case

I have a little piece of code, which works well with execute_async_script / execute_js_script.

Comment 4

[Jonathan Griffin (:jgriffin)]

execute_script returns whatever the executed script does, e.g., 

  execute_script("return 3;") will return 3.

I think what we want to do is to collect the results of any assertions made in the script, and return it as an extra property to the result here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#609

Then, we can still return the original value to the caller, but inspect the the assertions and add test failures there (by calling _handle_error if needed).

Comment 5

[mathieu bultel:mbu]

Ok, thx for your answer.

My fix do approximately what you describe, but in my unit tests, i have made a test for execute_script() like this :
execute_script("ok(2 == 1, 'test for ok()');")

I'v expected a failure with a response like that : 
{u'failed': 1, u'passed': 0, u'failures': [{u'diag': u'false was false, expected true', u'name': u'test for ok()'}]}

But i always have "None" in the response. 

I've probably missed something...

Comment 6

[Jonathan Griffin (:jgriffin)]

Feel free to attach a patch, if you have one, it might be easier to discuss.

Comment 7

[mathieu bultel:mbu]

Ok, i'll attach a patch soon .. Today, i tried to fix quickly #886741

Comment 8

[Jonathan Griffin (:jgriffin)]

Not actively working on this, since it's not currently blocking anything.

Comment 9

[David Burns :automatedtester]

this is not something we care about anymore since we dont do JS assertions anywhere anymore. We can reraise if this becomes an issue