Need ability to skip each test on a given conditions

RESOLVED FIXED in Firefox 40

Status

Testing
XPCShell Harness
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({dev-doc-complete})

unspecified
mozilla40
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8587861 [details] [diff] [review]
WIP v1
Assignee: nobody → hiikezoe
(Assignee)

Updated

3 years ago
Blocks: 937740
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 == '...')".
(Assignee)

Updated

3 years ago
Depends on: 1154111
(Assignee)

Comment 3

3 years ago
(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

3 years ago
(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.
(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.
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8593726 [details] [diff] [review]
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
Attachment #8587861 - Attachment is obsolete: true
Attachment #8593726 - Flags: review?(ted)
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!
Attachment #8593726 - Flags: review?(james)
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.
Attachment #8593726 - Flags: review?(james) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8595049 [details] [diff] [review]
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.
Attachment #8593726 - Attachment is obsolete: true
Attachment #8593726 - Flags: review?(ted)
Attachment #8595049 - Flags: review?(ted)
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.)
Attachment #8595049 - Flags: review?(ted) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8598401 [details] [diff] [review]
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!
Attachment #8595049 - Attachment is obsolete: true
Attachment #8598401 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/935efdab9ffc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/935efdab9ffc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40

Updated

3 years ago
Depends on: 1159676

Comment 15

3 years ago
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.
Keywords: dev-doc-needed
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.
Flags: needinfo?(hiikezoe)
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!
Keywords: dev-doc-needed → dev-doc-complete
Also, added a link to this to the "Other" section on Firefox 40 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/40
(Assignee)

Comment 19

2 years ago
(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!
Flags: needinfo?(hiikezoe)
You need to log in before you can comment on or make changes to this bug.