Closed Bug 820154 Opened 7 years ago Closed 6 years ago

Implement todo() in marionette-simpletest

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: jgriffin, Assigned: parkouss)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
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
Flags: needinfo?(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
Flags: needinfo?(jgriffin)
(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.
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 ?
I assume the solution is correct fo you - I try my luck !
Attachment #8436881 - Flags: review?(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
Attachment #8436881 - Flags: review?(jgriffin) → review-
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 ?
Attachment #8436881 - Attachment is obsolete: true
Attachment #8438595 - Flags: review?(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.
Attachment #8438595 - Flags: review?(jgriffin) → review-
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 ?
Flags: needinfo?(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.
Flags: needinfo?(jgriffin)
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 !
Attachment #8438595 - Attachment is obsolete: true
Attachment #8443245 - Flags: review?(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
Attachment #8443245 - Flags: review?(jgriffin) → review-
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 ?
Attachment #8443245 - Attachment is obsolete: true
Attachment #8444949 - Flags: review?(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.
Depends on: 956739
As I suspected, bug 956739 bitrotted this a bit.  Julien, do you mind updating the patch?
Flags: needinfo?(j.parkouss)
Assignee: nobody → j.parkouss
Flags: needinfo?(j.parkouss)
That's OK, i will look at it soon.
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.
Attachment #8444949 - Flags: review?(jgriffin) → review-
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 ?
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?
Flags: needinfo?(cmanchester)
(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!
Flags: needinfo?(cmanchester)
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.
Attachment #8444949 - Attachment is obsolete: true
Attachment #8471102 - Flags: review?(jgriffin)
This must be better - I corrected the KeyError: 'failed' error that was breaking the push try.
Attachment #8471102 - Attachment is obsolete: true
Attachment #8471102 - Flags: review?(jgriffin)
Attachment #8472342 - Flags: review?(jgriffin)
Thanks Julien.  hg is having some problems today, I might not be able to post this to try right away.
Comment on attachment 8472342 [details] [diff] [review]
Implement todo() in marionette-simpletest

Try looks good, thanks!
Attachment #8472342 - Flags: review?(jgriffin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/63cdf0eb0975
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
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.
Flags: needinfo?(jgriffin)
I'll release a new package tomorrow (Wed).
Flags: needinfo?(jgriffin)
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?
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.
You need to log in before you can comment on or make changes to this bug.