1150822 - Need ability to skip each test on a given conditions

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Each test file can be skipped with 'skip-if' (or 'run-if') in xpcshell.ini but each test function in the file can not be skipped for now.

I am now thinking something like the following:

add_test(function test_function() {
...
}, { skip_if: () => MOZINFO.os == "linux" });

The arrow function here is needed to make log meaningful because the expression (MOZINFO.os == "linux") has already evaluated when the test is run.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP v1

Comment 2

[(not currently active) Ted Mielczarek]

Two notes:
1) The mozlog changes here should probably be split off to their own bug.
2) I've thought for a while that it would be nice to expose mozinfo to JS in all of our test harnesses. We have a lot of tests (most of which predate test manifests) that use ugly hacks to detect what platform they're running on, it would be much nicer if they could just say "if (mozinfo.os == '...')".

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Two notes:
> 1) The mozlog changes here should probably be split off to their own bug.

Filed bug 1154111.

> 2) I've thought for a while that it would be nice to expose mozinfo to JS in
> all of our test harnesses. We have a lot of tests (most of which predate
> test manifests) that use ugly hacks to detect what platform they're running
> on, it would be much nicer if they could just say "if (mozinfo.os == '...')".

As you already know, bug 1150818 and bug 1153126 have been opend for importing mozinfo.

Comment 4

[:Paolo Amadini]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> 2) I've thought for a while that it would be nice to expose mozinfo to JS in
> all of our test harnesses. We have a lot of tests (most of which predate
> test manifests) that use ugly hacks to detect what platform they're running
> on, it would be much nicer if they could just say "if (mozinfo.os == '...')".

I get that the advantage of mozinfo over the existing methods is that this would match the conditions checked by the test manifests? If so, this seems definitely the way to go.

A note on the syntax for add_test and add_task: while looking at bug 937740, I was thinking it would be much better to see the test skip condition (as well as other possible metadata) _before_ the test function, especially important if the function is long.

add_task({
  skip_if: () => mozinfo.os == "linux",
}, function* test_function() {
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ...
  // ... well, I think I made my point :-)
});

This also facilitates other extensions like:

add_task({
  description: "I'm not a fan of descriptions but someone might find them useful.",
  fail_if: () => mozinfo.os == "b2g",
  fail_if_description: "This is expected to fail on B2G until bug 123456 is resolved.",
}, function* () {
  // ...
});

Last note, I think there is no need to implement "run_if", there is no equivalent in the test manifests and I can see how this would allow the same condition being expressed in two ways in two different tests.

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to :Paolo Amadini from comment #4)
> I get that the advantage of mozinfo over the existing methods is that this
> would match the conditions checked by the test manifests? If so, this seems
> definitely the way to go.

Yes, the mozinfo object we'd be exposing would have the same keys as are available in manifest conditions, which is great for developer utility.

> A note on the syntax for add_test and add_task: while looking at bug 937740,
> I was thinking it would be much better to see the test skip condition (as
> well as other possible metadata) _before_ the test function, especially
> important if the function is long.
> 
> add_task({
>   skip_if: () => mozinfo.os == "linux",
> }, function* test_function() {

This is a good point and probably isn't hard to do.

> Last note, I think there is no need to implement "run_if", there is no
> equivalent in the test manifests and I can see how this would allow the same
> condition being expressed in two ways in two different tests.

Right, we actually removed this from manifests in bug 958147.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to :Paolo Amadini from comment #4)

> add_task({
>   description: "I'm not a fan of descriptions but someone might find them
> useful.",
>   fail_if: () => mozinfo.os == "b2g",
>   fail_if_description: "This is expected to fail on B2G until bug 123456 is
> resolved.",
> }, function* () {
>   // ...
> });

Thanks for nice advices! This looks much better.
I will revise my patch.
About "fail_if" I am going to open a followup bug.

> Last note, I think there is no need to implement "run_if", there is no
> equivalent in the test manifests and I can see how this would allow the same
> condition being expressed in two ways in two different tests.

Right, run_if has been already removed in my local patch.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add skip_if directive for add_test and add_task

Actually skip_if is not useful until mozinfo is come into xpcshell but we can land it now without mozinfo. So I am going to request review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=367abde8a9b2

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8593726 [details] [diff] [review]
Add skip_if directive for add_test and add_task

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

Overall this looks good, but I want jgraham to comment on the structured logging bit.

::: testing/xpcshell/head.js
@@ +1381,4 @@
>   */
> +function add_task(funcOrProperties, func) {
> +  if (typeof funcOrProperties == "function") {
> +    _gTests.push([funcOrProperties, { _isTask: true }]);

Is there a specific reason you reordered the elements of the array being pushed in gTests?

@@ +1422,5 @@
> +        _testLogger.testStatus(_TEST_NAME,
> +                               _gRunningTest.name,
> +                               "SKIP",
> +                               "SKIP",
> +                               _message);

I'm not 100% sure this will do the right thing in our log handling code. Flagging jgraham for additional review on this bit.

::: testing/xpcshell/selftest.py
@@ +560,5 @@
>          self.assertEquals(0, self.x.todoCount)
>          self.assertInLog(TEST_PASS_STRING)
>          self.assertNotInLog(TEST_FAIL_STRING)
>  
> +    def testSkipForAddTest(self):

Thanks for writing selftests!

Comment 9

[James Graham [:jgraham]]

Comment on attachment 8593726 [details] [diff] [review]
Add skip_if directive for add_test and add_task

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

I just reviewed the bit that ted specifically asked about.

::: testing/xpcshell/head.js
@@ +1422,5 @@
> +        _testLogger.testStatus(_TEST_NAME,
> +                               _gRunningTest.name,
> +                               "SKIP",
> +                               "SKIP",
> +                               _message);

I think this ought to work since bug 1154111, and should it interfere with any downstream tools I think those need to be fixed.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add skip_if directive for add_test and add_task v2

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Comment on attachment 8593726 [details] [diff] [review]
> Add skip_if directive for add_test and add_task
> 
> Review of attachment 8593726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks good, but I want jgraham to comment on the structured
> logging bit.
> 
> ::: testing/xpcshell/head.js
> @@ +1381,4 @@
> >   */
> > +function add_task(funcOrProperties, func) {
> > +  if (typeof funcOrProperties == "function") {
> > +    _gTests.push([funcOrProperties, { _isTask: true }]);
> 
> Is there a specific reason you reordered the elements of the array being
> pushed in gTests?

Ah, this is unintentional.
Fixed.

Pushed on try for the safety.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=678eac478628
Looks good.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 8595049 [details] [diff] [review]
Add skip_if directive for add_test and add_task v2

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

Thanks for the patch and for the selftests!

::: testing/xpcshell/head.js
@@ +1328,5 @@
>  /**
>   * Add a test function which is a Task function.
>   *
> + * @param funcOrProperties
> + *        A genrator function to be run or an object represents test

typo: "generator"

::: testing/xpcshell/selftest.py
@@ +553,5 @@
> +        """
> +        self.writeFile("test_skip.js", """
> +add_test({
> +  skip_if: () => true,
> +}, function test_shoud_be_skipped() {

typo: "should"

@@ +587,5 @@
> +        self.assertEquals(0, self.x.failCount)
> +        self.assertEquals(0, self.x.todoCount)
> +        self.assertInLog(TEST_PASS_STRING)
> +        self.assertNotInLog("TEST-SKIP")
> +        self.assertNotInLog(TEST_FAIL_STRING)

I wonder if it wouldn't be nice to factor out a method like "assertSingleTestPass" to capture all these checks we have in all these tests. (You don't have to fix that to land this patch.)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add skip_if directive for add_test and add_task v3

> 
> @@ +587,5 @@
> > +        self.assertEquals(0, self.x.failCount)
> > +        self.assertEquals(0, self.x.todoCount)
> > +        self.assertInLog(TEST_PASS_STRING)
> > +        self.assertNotInLog("TEST-SKIP")
> > +        self.assertNotInLog(TEST_FAIL_STRING)
> 
> I wonder if it wouldn't be nice to factor out a method like
> "assertSingleTestPass" to capture all these checks we have in all these
> tests. (You don't have to fix that to land this patch.)

Opened bug 1159108.

Thanks!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/935efdab9ffc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/935efdab9ffc

Comment 15

[:Paolo Amadini]

Added the dev-doc-needed keyword, most probably the changes related to this testing framework API are self-contained to this page:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

The exact syntax from the final patch might differ from the bug comments, the selftests in the final patch are a realiable source instead.

Comment 16

[Eric Shepherd [:sheppy]]

I'm working on documenting this now. Do any of the conditions other than skip_if work? I don't think so (this patch doesn't seem to include them) but want to check.

Comment 17

[Eric Shepherd [:sheppy]]

I've made the following changes:

In https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#XPCShell_test_utility_functions -- added text to add_test() and add_task() mentioning the added new parameter and linking to the section I've added documenting that in more detail.

Added new section https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Conditional_test_functions to describe these test conditions.

Updated section https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Adding_conditions_to_a_test to mention this capability with a link to the above, and also fixed a wording problem.

A review of these changes would be fantastic; if there are problems, let me know and switch back to dev-doc-needed. Thanks!

Comment 18

[Eric Shepherd [:sheppy]]

Also, added a link to this to the "Other" section on Firefox 40 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/40

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Eric Shepherd [:sheppy] from comment #16)
> I'm working on documenting this now. Do any of the conditions other than
> skip_if work? I don't think so (this patch doesn't seem to include them) but
> want to check.

Right. We have only skip_if function. 
And the document is perfect!

Thanks for the documentation work!