Closed Bug 1380470 Opened 4 years ago Closed 4 years ago

Allow tasks to be skipped or only one task to run in all suites

Categories

(Testing :: General, enhancement)

Product:
Testing
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(3 files)

Bug 1380470 - Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1380470 - Part 2 - Add add_task().skip() and add_task().only() for mochitest-chrome unit tests.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1380470 - Part 3 - Add add_task().skip() and add_task().only() for XPCShell unit tests.
59 bytes, text/x-review-board-request
ahal
: review+
Details 
This was probably a mind-fart from Mark, but in # fx-team he asked: http://logs.glob.uno/?c=fx-team#c294091

It reminded of this itch that needed scratching really badly ;-)

Anyway, this set of patches implements `add_task(async function someTest() {}).skip()` and `add_task(async function someTest() {}).only()`, for Mochitest-chrome, Mochitest-browser and XPCShell, with the following semantics;

* `.skip()` makes sure that the task in question is not run as part of the suite. This will and should be made very clear in the log output as well.
* `.only()` makes sure that only the task in question is run, thus all the other tasks in that suite will be skipped. If `.only()` is called multiple times, it will run the last one on the stack, IOW: the last task on which `.only()` was called.

The motivation behind this feature is to stimulate the adoption of Test Driven Development (TDD) and intermittent failure research work by allowing more granular control of what test gets run during your debugging process.
In large, growing test suites it's more likely that you'll be wishing to skip a certain test case because it takes a long time or has nothing to do with the failure case or rather to check if it pollutes the behavior of other tests.
In large, growing suites, it's also more likely that you'll be wishing for a convenient way to focus on a single test case, instead of the suite as whole. Example: extended feature development.
Comment on attachment 8885916 [details]
Bug 1380470 - Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests.

https://reviewboard.mozilla.org/r/156702/#review161840

I haven't touched JavaScript or the test harnesses in ages. While these changes do seem reasonable, I don't feel confident giving review on this series. So cancelling review.

I'm not sure the best person to ask for review. Try asking around in #ateam.
Attachment #8885916 - Flags: review?(gps)
Attachment #8885917 - Flags: review?(gps)
Attachment #8885918 - Flags: review?(gps)
Joel, who would be the right person the review these test harness changes?
Flags: needinfo?(jmaher)
:ahal will be a good reviewer for these patches
Flags: needinfo?(jmaher)
Sweet, thanks Joel... and Andrew ;-)
Comment on attachment 8885916 [details]
Bug 1380470 - Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests.

https://reviewboard.mozilla.org/r/156702/#review162870

Thanks, this looks good! This is mainly meant to be used for local debugging right? I'm worried that someone is going accidentally check in a .only() and we'll never notice, especially since log results get buffered on mochitest (and only dumped when the test fails).

I haven't looked at the other commits yet, but I'm assuming I'll have similar concerns there, so I'm holding off reviewing for now.

::: testing/mochitest/browser-test.js:762
(Diff revision 2)
>          let PromiseTestUtils = this.PromiseTestUtils;
>          this.Task.spawn(function*() {
>            let task;
>            while ((task = this.__tasks.shift())) {
> +            if (task.__skipMe || (this.__runOnlyThisTask && task != this.__runOnlyThisTask)) {
> +              this.SimpleTest.info("Skipping test " + task.name);

You should use:

    this.structuredLogger.testStatus(this.currentTest.path, task.name, "SKIP");

I'm not sure how the log formatters are going print that. We could also print this as a warning to be extra visible:

    this.structuredLogger.warning("Skipping test " + task.name);

At least with this information in the structured log we'll be able to write a tool that can easily spit out all the skipped subtests, but even then I doubt people would run that very often. When running locally, be sure to test this with `--quiet` as that's how the logs will look in automation.

::: testing/mochitest/browser-test.js:1081
(Diff revision 2)
>  }
> +
> +function decorateTaskFn(fn) {
> +  fn = fn.bind(this);
> +  fn.skip = () => fn.__skipMe = true;
> +  fn.only = () => this.__runOnlyThisTask = fn;

Maybe this should raise if `this.__runOnlyThisTask != null`?
Attachment #8885916 - Flags: review?(ahalberstadt) → review-
Comment on attachment 8885916 [details]
Bug 1380470 - Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests.

https://reviewboard.mozilla.org/r/156702/#review162870

> You should use:
> 
>     this.structuredLogger.testStatus(this.currentTest.path, task.name, "SKIP");
> 
> I'm not sure how the log formatters are going print that. We could also print this as a warning to be extra visible:
> 
>     this.structuredLogger.warning("Skipping test " + task.name);
> 
> At least with this information in the structured log we'll be able to write a tool that can easily spit out all the skipped subtests, but even then I doubt people would run that very often. When running locally, be sure to test this with `--quiet` as that's how the logs will look in automation.

We could also make sure that `WARNING` messages don't get buffered so we'd always see this even with `--quiet`.
(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Thanks, this looks good! This is mainly meant to be used for local debugging
> right? I'm worried that someone is going accidentally check in a .only() and
> we'll never notice, especially since log results get buffered on mochitest
> (and only dumped when the test fails).

Yeah, local debugging only! I'll send a PSA to dev-platform that stresses this detail for reviewers.

> You should use:
> 
>     this.structuredLogger.testStatus(this.currentTest.path, task.name,
> "SKIP");
> 
> I'm not sure how the log formatters are going print that. We could also
> print this as a warning to be extra visible:
> 
>     this.structuredLogger.warning("Skipping test " + task.name);
> 
> At least with this information in the structured log we'll be able to write
> a tool that can easily spit out all the skipped subtests, but even then I
> doubt people would run that very often. When running locally, be sure to
> test this with `--quiet` as that's how the logs will look in automation.

Cool, I looked into the StructureLogger infra and looks quite straightforward. I hope I did it right! (It shows skipped tests when run with --quiet too now)

> Maybe this should raise if `this.__runOnlyThisTask != null`?

That would alter its behavior. This utility is indeed intended to be used whilst debugging and coding in general, not in production, so executing the last task set with `.only()` would make sense and is also how it behaves in other test suites, like Mocha.
(In reply to Mike de Boer [:mikedeboer] from comment #16)
> is also how it behaves in other test suites, like Mocha.

Not that this is in any way important, btw. Just mentioning it. I think that it's right choice ergonomically.
(In reply to Mike de Boer [:mikedeboer] from comment #17)
> (In reply to Mike de Boer [:mikedeboer] from comment #16)
> > is also how it behaves in other test suites, like Mocha.
> 
> Not that this is in any way important, btw. Just mentioning it. I think that
> it's right choice ergonomically.

That's fine, feel free to drop the issue. I was just wondering.
Comment on attachment 8885916 [details]
Bug 1380470 - Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests.

https://reviewboard.mozilla.org/r/156702/#review162982

I didn't even think of deavtivating the buffering that way, but this works too! I was thinking of a more general purpose solution that ensured WARNING and ERROR were never buffered. But no need to block on that when this will work perfectly well too.
Attachment #8885916 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8885917 [details]
Bug 1380470 - Part 2 - Add add_task().skip() and add_task().only() for mochitest-chrome unit tests.

https://reviewboard.mozilla.org/r/156704/#review162992
Attachment #8885917 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8885918 [details]
Bug 1380470 - Part 3 - Add add_task().skip() and add_task().only() for XPCShell unit tests.

https://reviewboard.mozilla.org/r/156706/#review163004

::: testing/xpcshell/example/unit/xpcshell.ini:48
(Diff revision 3)
> +[test_tasks_skip.js]
> +[test_tasks_skipall.js]

Does this manifest get run anywhere? I picked a random xpcshell task and couldn't find it listed in the logs. You might want to add these tests to `testing/xpcshell/selftest.py` instead.

::: testing/xpcshell/head.js
(Diff revision 3)
> -/**
> - * Add a test function to the list of tests that are to be run asynchronously.
> - *
> - * @param funcOrProperties
> - *        A function to be run or an object represents test properties.
> - *        Supported properties:
> - *          skip_if : An arrow function which has an expression to be
> - *                    evaluated whether the test is skipped or not.
> - * @param func
> - *        A function to be run only if the funcOrProperies is not a function.
> - *
> - * Each test function must call run_next_test() when it's done. Test files
> - * should call run_next_test() in their run_test function to execute all
> - * async tests.
> - *
> - * @return the test function that was passed in.
> - */
> -var _gTests = [];
> -function add_test(funcOrProperties, func) {
> -  if (typeof funcOrProperties == "function") {
> -    _gTests.push([{ _isTask: false }, funcOrProperties]);
> -  } else if (typeof funcOrProperties == "object") {
> -    funcOrProperties._isTask = false;
> -    _gTests.push([funcOrProperties, func]);
> -  } else {
> -    do_throw("add_test() should take a function or an object and a function");
> -  }
> -  return func;
> -}
> -add_test.only = _add_only.bind(undefined, add_test);
> -add_test.skip = _add_skip.bind(undefined, add_test);

I don't think we can remove this. There are tests to make sure this works [1] and many uses of this function scattered around the tree [2].

[1] https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/selftest.py#361
[2] https://dxr.mozilla.org/mozilla-central/search?q=path:xpcshell%20add_test
Attachment #8885918 - Flags: review?(ahalberstadt) → review-
(In reply to Andrew Halberstadt [:ahal] from comment #21)
> Does this manifest get run anywhere? I picked a random xpcshell task and
> couldn't find it listed in the logs. You might want to add these tests to
> `testing/xpcshell/selftest.py` instead.

You're right! I need to add this to selftest.py too. I think I'll leave the examples, because they might for ppl to look at.

> I don't think we can remove this. There are tests to make sure this works
> [1] and many uses of this function scattered around the tree [2].

Crap, I misread `add_test` as `add_task` and removed it because I thought it was duplicated/ dead code. Brrr, I'll put it back.
Comment on attachment 8885918 [details]
Bug 1380470 - Part 3 - Add add_task().skip() and add_task().only() for XPCShell unit tests.

https://reviewboard.mozilla.org/r/156706/#review163542

Thanks for the cleanup/docs/test!
Attachment #8885918 - Flags: review?(ahalberstadt) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a039da5847b
Part 1 - Add add_task().skip() and add_task().only() for mochitest-browser unit tests. r=ahal
https://hg.mozilla.org/integration/autoland/rev/d8e743897f70
Part 2 - Add add_task().skip() and add_task().only() for mochitest-chrome unit tests. r=ahal
https://hg.mozilla.org/integration/autoland/rev/051d1c772798
Part 3 - Add add_task().skip() and add_task().only() for XPCShell unit tests. r=ahal
https://hg.mozilla.org/mozilla-central/rev/8a039da5847b
https://hg.mozilla.org/mozilla-central/rev/d8e743897f70
https://hg.mozilla.org/mozilla-central/rev/051d1c772798
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1421604
See Also: → 1446304
Depends on: 1526445
You need to log in before you can comment on or make changes to this bug.