820154 - Implement todo() in marionette-simpletest

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

JS tests need a way of flagging expected failures.  In mochitest, we do this using todo(); we should implement something similar in marionette-simpletest.js.

Comment 1

[Julien Pagès (:parkouss)]

Hey Jonathan,

I'd like to take the bug. I looked at it and found a solution, but I'm not sure it suit your needs:

In js test file, you add a line like this to mark the test as "todo":

MARK_AS_TODO();

then there is 2 possibilities for the report:

1. at least one test inside the file failed (example: ok(1 == 2, "this will fail")). report will show:

> Ran 1 test in 0.363s
> 
> OK (expected failures=1)
> 
> SUMMARY
> -------
> passed: 0
> failed: 0
> todo: 1

2. none of the tests inside the file failed (example: ok(2 == 2, "this will success")). report will show:

> Ran 1 test in 0.360s
> 
> OK (unexpected successes=1)
> 
> SUMMARY
> -------
> passed: 0
> failed: 1 (unexpected sucesses: 1)
> todo: 0
> 
> FAILED TESTS
> -------
> test_simpletest_todo.js

Is this the intended behavior ? I think it is the same behaviour for "todo" python tests in marionette [1].

If you like the idea, I can send a patch. :)

[1] http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#1019

Comment 2

[Jonathan Griffin (:jgriffin)]

Hello!

Yes, that's exactly the effect we want to have.  To maintain consistency with mochitest, we should implement this so that todo() is used in place of ok(), rather than wrapping with MARK_AS_TODO().  

See e.g., http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug598643.html?force=1#70

Comment 3

[Julien Pagès (:parkouss)]

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> Hello!
> 
> Yes, that's exactly the effect we want to have.  To maintain consistency
> with mochitest, we should implement this so that todo() is used in place of
> ok(), rather than wrapping with MARK_AS_TODO().  
> 
> See e.g.,
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/
> test_bug598643.html?force=1#70

Ah, ok. Well MARK_AS_TODO() was a single call on top of the js file, unlike tour todo() description.

Have a todo() working like ok() is a bit more complicated for the report (with runtests.py). Here's why:

- one js file may contain multiple calls to ok(), is(), ... and todo().
- one js file execution is counted as one and only one test in the report, without taking in account the number of ok(), is(), ... and todo() calls.

So what will I report if I have one todo() and one fail() in the same file ? or another combination ?

If I make a comparison with python tests, I can say that a python test method is like a js test file here, and that assert calls in python are like ok(), is() functions. But in python you mark a method as todo, not an assert call - and so i wanted to mark a js test file as todo.

But I don't know mochitest - maybe you could help me more with the report behaviour if you want a todo() call like this.

Comment 4

[Julien Pagès (:parkouss)]

Rethinking about this, I may have a solution with the todo() call:

- if at the end of the test execution, we have at least one fail, then report a fail.
- else if we have at least one todo, and they're all "expected failures", report a todo
- else if we have at least one todo and at least one of them is an "unexpected success", then report a unexpected success fail

Is that an appropriate solution ?

Comment 5

[Julien Pagès (:parkouss)]

Attachment: Bug 820154 - Implement todo() in marionette-simpletest

I assume the solution is correct fo you - I try my luck !

Comment 6

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8436881 [details] [diff] [review]
Bug 820154 - Implement todo() in marionette-simpletest

Review of attachment 8436881 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is close!

For todo() assertions, we want them to be reported with the "passString" of this.TEST_KNOWN_FAIL, rather than this.TEST_PASS and a failString of "TEST-UNEXPECTED-PASS" (there's no constant right now in the file for that) instead of test.TEST_UNEXPECTED_FAIL.  Otherwise, I think your logic for handling things looks right.

It would be good to add a test of todo() to test_simpletest_pass.js.

::: testing/marionette/client/marionette/marionette_test.py
@@ +512,5 @@
> +                    else:
> +                        fallback_name = "got false, expected true"
> +                    name = fallback_name if failure.get('name') is None else failure['name']
> +                    if is_unexpected:
> +                        name = "[unexpected] %s" % name

We don't need prefix the name with [unexpected]; that should be made clear by TEST-UNEXPECTED-*.

::: testing/marionette/marionette-simpletest.js
@@ +54,5 @@
>      let diag = pass ? this.repr(a) + " should not equal " + this.repr(b)
>                      : "didn't expect " + this.repr(a) + ", but got it";
> +    this.addTest(pass, name, passString, failString, diag);
> +  },
> +  

nit: extra whitespace here

Comment 7

[Julien Pagès (:parkouss)]

Attachment: Implement todo() in marionette-simpletest

I followed your instructions and updated the patch. The test of todo() in test_simpletest_pass.js mark the test as todo (obviously) when running the unittests. Is that ok ?

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8438595 [details] [diff] [review]
Implement todo() in marionette-simpletest

Review of attachment 8438595 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks mostly ok.  Basically, each JS test is capable of having exactly one value, regardless of how many is/ok/todo calls are inside of it, and fail will supersede todo, which we want.  There's only one change I think we really want:

::: testing/marionette/client/marionette/marionette_test.py
@@ +517,5 @@
>                  self.assertEqual(0, results['failed'],
>                                   '%d tests failed:\n%s' % (results['failed'], '\n'.join(fails)))
> +                if results['unexpectedSuccess'] > 0:
> +                    raise _UnexpectedSuccess("%d unexpected successes:\n%s" %
> +                                             (results['unexpectedSuccess'], '\n'.join(fails)))

For each unexpectedSuccess, we should print out a line like:

TEST-UNEXPECTED-SUCCESS | test_foo.js | description

similar to what we do for failing tests.

We'd also like a similar line, but TEST-KNOWN-FAIL, for expectedFailures.

Comment 9

[Julien Pagès (:parkouss)]

If I understand well, a JS test file like that:

> todo(2 == 2, "todo unexpected");
> todo(1 == 2, "todo expected");

Must have an output like:

> (marionette_auto_venv)[jp@parkouss marionette]$ python runtests.py tests/unit/test_simpletest_pass.js --binary ../../../../obj-x86_64-unknown-linux-gnu/dist/bin/firefox-bin
> starting httpd
> running webserver on http://127.0.0.1:40920/
> TEST-START test_simpletest_pass.js
> /home/jp/dev/mozilla/mozilla-central/testing/marionette/client/marionette/tests/unit/test_simpletest_pass.js, runTest (marionette_test.MarionetteJSTestCase) ...
> 1 unexpectedSuccesses:
> TEST-UNEXPECTED-SUCCESS | test_simpletest_pass.js | true was an unexpected success | todo unexpected
> 1 expectedFailures:
> TEST-KNOWN-FAIL | test_simpletest_pass.js | false was an unexpected success | todo expected
> 
> ----------------------------------------------------------------------
> Ran 1 test in 0.421s
> 
> OK (unexpected successes=1)
> 
> SUMMARY
> -------
> passed: 0
> failed: 1 (unexpected sucesses: 1)
> todo: 0
> 
> FAILED TESTS
> -------
> test_simpletest_pass.js

ie, with one line like "TEST-UNEXPECTED-SUCCESS |" or the like for each todo call in the test file (may be more than one for one test file). Is that right ?

Comment 10

[Jonathan Griffin (:jgriffin)]

Yes, that's right.  Each todo() should cause one line of output, either TEST-UNEXPECTED-SUCCESS or TEST-KNOWN-FAIL, as your output shows.

Comment 11

[Julien Pagès (:parkouss)]

Attachment: Implement todo() in marionette-simpletest

Hey ! I added support to print lines as required. The comment #9 shows an output of what it does. I had to add some behaviour in marionette/runner/base.py, I hope it is ok !

Comment 12

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1f4f41c188e8

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8443245 [details] [diff] [review]
Implement todo() in marionette-simpletest

Review of attachment 8443245 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like test_simpletest_fail.js fails with this patch:  https://tbpl.mozilla.org/php/getParsedLog.php?id=42280077&tree=Try&full=1#error0

Comment 14

[Julien Pagès (:parkouss)]

Attachment: Implement todo() in marionette-simpletest

Thank you for the push try. I am able to correct the mistakes:

TEST-UNEXPECTED-FAIL | test_simpletest_fail.js | AssertionError: no tests run
TEST-UNEXPECTED-FAIL | test_simpletest_sanity.py test_simpletest_sanity.SimpletestSanityTest.test_is | KeyError: 'failed'
TEST-UNEXPECTED-FAIL | test_simpletest_sanity.py test_simpletest_sanity.SimpletestSanityTest.test_isnot | KeyError: 'failed'
TEST-UNEXPECTED-FAIL | test_simpletest_sanity.py test_simpletest_sanity.SimpletestSanityTest.test_ok | KeyError: 'failed'

I also moved todo() tests in test_simpletest_sanity, as it seemed more appropriate.

For the last error (no module named marionette_test on B2G Desktop Linux x64 Opt), I had to write the logic in runtests.py rather than in runner/base.py - I had issues with lines like "from marionette import marionette_test" in runner/base.py (it seemed to me that the file marionette/marionette.py was loaded instead of marionette/__init__.py)

This new patch should pass the push try now.Could you try another one, if the changes are correct for you ?

Comment 15

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b82414a2ed95

Comment 16

[Jonathan Griffin (:jgriffin)]

This looks good and try was green, but it's quite possibly going to be bitrotted by bug 956739, which should land shortly.  Let's see if we need to refactor this a bit after that lands.

Comment 17

[Jonathan Griffin (:jgriffin)]

As I suspected, bug 956739 bitrotted this a bit.  Julien, do you mind updating the patch?

Comment 18

[Julien Pagès (:parkouss)]

That's OK, i will look at it soon.

Comment 19

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8444949 [details] [diff] [review]
Implement todo() in marionette-simpletest

Review of attachment 8444949 [details] [diff] [review]:
-----------------------------------------------------------------

R- only because this patch no longer cleanly applies.

Comment 20

[Julien Pagès (:parkouss)]

Hi Jonathan,

I'm reworking on this, but code base has changed a lot! I assume now I have to use a logger (from mozlog.structured.structuredlog.get_default_logger) to log TEST-KNOW-FAIL and TEST-UNEXPECTED-SUCCESS lines.

But I can not write a TEST-KNOW-FAIL line with this. According to the docstring:

  test_status
      test - ID for the test
      subtest - Name of the subtest
      status [PASS | FAIL | TIMEOUT | NOTRUN] - test status
      expected [As for status] - Status that the subtest was expected to get,
                                 or absent if the subtest got the expected status

I though I have to write something (in marionette_test.py - http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#505) like this for expected failures:

logger.test_status(self.test_name, name, 'FAIL', message=diag)

Unfortunately, this print TEST-UNEXPECTED-FAIL instead of TEST-KNOW-FAIL. But according to the docstring, expected parameter must be absent if the test got the expected status - So I expected a TEST-KNOW-FAIL printed here...
I also played with the "expected" parameter, but I'm unable to get a TEST-KNOW-FAIL printed, whatever I do.

What have I missed ?

Comment 21

[Jonathan Griffin (:jgriffin)]

It's possible this is a bug in structured logging, but I'm not sure.  Chris may be able to help out here; Chris, how should Julien log known failures from JS tests in Marionette?

Comment 22

[Chris Manchester (limited bugmail, email directly)]

(In reply to Julien Pagès from comment #20)
> Hi Jonathan,
> 
> I'm reworking on this, but code base has changed a lot! I assume now I have
> to use a logger (from mozlog.structured.structuredlog.get_default_logger) to
> log TEST-KNOW-FAIL and TEST-UNEXPECTED-SUCCESS lines.
> 
> But I can not write a TEST-KNOW-FAIL line with this. According to the
> docstring:
> 
>   test_status
>       test - ID for the test
>       subtest - Name of the subtest
>       status [PASS | FAIL | TIMEOUT | NOTRUN] - test status
>       expected [As for status] - Status that the subtest was expected to get,
>                                  or absent if the subtest got the expected
> status
> 
> I though I have to write something (in marionette_test.py -
> http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/
> marionette/marionette_test.py#505) like this for expected failures:
> 
> logger.test_status(self.test_name, name, 'FAIL', message=diag)
> 
> Unfortunately, this print TEST-UNEXPECTED-FAIL instead of TEST-KNOW-FAIL.
> But according to the docstring, expected parameter must be absent if the
> test got the expected status - So I expected a TEST-KNOW-FAIL printed here...
> I also played with the "expected" parameter, but I'm unable to get a
> TEST-KNOW-FAIL printed, whatever I do.
> 
> What have I missed ?

The docstring above is for the data format, not the methods on the structured logger, so while consumers parsing the output of the logger shouldn't find an "expected" field in test status message objects that get logged if nothing went wrong in the test, the test_status method itself populates a default of "PASS" for its expected parameter, which I think explains what you've observed. To log a known fail, I would do something like:

logger.test_status(self.test_name, name, status='FAIL', expected='FAIL', message=diag)

Which should have "TEST-FAIL" at the beginning of its line according to the current implementation of the formatter. I think you've identified the right place to do it in marionette_test.py.

Sorry about the bit rot and the misleading docstring, please feel free to ping me if you have any questions about logging as you proceed. Thanks!

Comment 23

[Julien Pagès (:parkouss)]

Attachment: Implement todo() in marionette-simpletest - 2

Thank you Chris for your explanations. :)

I assume that TEST-FAIL must now be printed instead of TEST-KNOW-FAIL when we see a todo test.

Comparing to the last patch, only marionette_test.py has changed, and changes for the runtests.py file were removed since it is no more needed.

Comment 24

[Jonathan Griffin (:jgriffin)]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bd13d288764e

Comment 25

[Julien Pagès (:parkouss)]

Attachment: Implement todo() in marionette-simpletest

This must be better - I corrected the KeyError: 'failed' error that was breaking the push try.

Comment 26

[Jonathan Griffin (:jgriffin)]

Thanks Julien.  hg is having some problems today, I might not be able to post this to try right away.

Comment 27

[Jonathan Griffin (:jgriffin)]

try: https://tbpl.mozilla.org/?tree=Try&rev=5d4bfef96144

Comment 28

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8472342 [details] [diff] [review]
Implement todo() in marionette-simpletest

Try looks good, thanks!

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63cdf0eb0975

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63cdf0eb0975

Comment 31

[Michael Vines [:m1] [:evilmachines]]

Hey :jgriffen, any idea when a new marionette_client package will be released with this patch?

We have a fun bug over here now where marionette_client 0.8.2 is incompatible with the new fields returned by marionette-simpletest.js::generate_results()

eg. we use 
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/requirements.txt#L1 to setup our venv, then run against a m-c version of Gecko

Same "KeyError: 'failed'" error as comment 25.

Comment 32

[Jonathan Griffin (:jgriffin)]

I'll release a new package tomorrow (Wed).

Comment 33

[Michael Vines [:m1] [:evilmachines]]

Awesome, thanks.  Will you update the https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/requirements.txt#L1 to match as well?  Want me to file a new bug?

Comment 34

[Jonathan Griffin (:jgriffin)]

https://pypi.python.org/pypi/marionette_client/0.8.3 released (see bug 1056977).  I'll file a separate bug to bump gaia-ui-tests.