proposal: Add amendments to browser.test API (#1051) - #1057
justinlulejian wants to merge 4 commits into
Conversation
| - `message` (string, optional) | ||
|
|
||
| **`browser.test.succeed(message)`** | ||
| Immediately marks the current test as passed, optionally with a custom message. This is helpful in cases where returning a Promise or `undefined` is less obvious than explicitly indicating success. |
There was a problem hiding this comment.
This differs from what we do in Safari/WebKit. browser.test.succeed(message) is just an alias for assertTrue(true, message). We have browser.test.notifyPass(message) as the "marks the current test as passed" method. IIRC, this is what Chrome and Firefox did too last I checked when we added these.
Should WebKit have succeed alias to notifyPass instead?
There was a problem hiding this comment.
In Chrome browser.test.notifyPass/browser.test.notifyFail are for the whole suite of browser.test.runTests, whereas browser.test.succeed in for the specific test case it's run in. For Safari, is browser.test.notifyPass/browser.test.notifyFail for the whole suite of tests or each individual test case? If the latter, then yes you could have browser.test.succeed/browser.test.fail just alias to browser.test.notifyPass/browser.test.notifyFail.
There was a problem hiding this comment.
In Chrome browser.test.notifyPass/browser.test.notifyFail are for the whole suite of browser.test.runTests
what about outside the context of runTests? how does notifyPass/notifyFail behave?
There was a problem hiding this comment.
IIRC, this is what Chrome and Firefox did too last I checked when we added these.
Firefox also does an alias: https://searchfox.org/firefox-main/source/toolkit/components/extensions/child/ext-test.js#299-305
There was a problem hiding this comment.
In Firefox we rarely use browser.test.succeed (either explicit assertion messages or we log, e.g. with browser.test.log). We use browser.test.fail to signal (unexpected) failures in the test when an assertXXX variant does not make more sense).
In wpt, with explicit control flow through async function in runTests, I don't think that we need a method to short-circuit the execution. It can result in strange control flow questions:
browser.test.runTests([
async function test_one() {
browser.test.assertEq(1, 1, "one is one");
browser.test.succeed("Got there");
browser.test.assertTrue(true, "What is this?"); // <---
},
async function test_two() {
browser.test.assertEq(2, 2, "two is two");
},
]);| - `message` (string, optional) | ||
|
|
||
| **`browser.test.fail(message)`** | ||
| Immediately marks the current test as failed, optionally with a custom message. This is helpful in cases where returning a Promise or `undefined` is less obvious than explicitly indicating failure. |
There was a problem hiding this comment.
Similar to https://github.com/w3c/webextensions/pull/1057/changes#r3724127353. We have browser.test.fail(message) as an alias for assertTrue(false, message). browser.test.notifyFail(message) is "marks the current test as failed" method.
Should WebKit have fail alias to notifyFail instead?
|
|
||
| ### Properties | ||
|
|
||
| **`browser.test.isUserGestureActive`** (boolean) |
There was a problem hiding this comment.
WebKit currently has isProcessingUserGesture. We will need to add support for the new name before we can adopt this in WPT.
There was a problem hiding this comment.
I had switched this since it was suggested as a good alternative. Chrome also implements it as isProcessingUserGesture. Do other browser have a strong position on this? Otherwise I'm happy saving the effort and keeping it as isProcessingUserGesture.
There was a problem hiding this comment.
i'm fine with isUserGestureActive. but as Tim said, we would need to adopt in WebKit and wait for that change to be picked up in Safari Tech Preview before using it in the wpt test
| Runs the provided function in the context of a user gesture. | ||
|
|
||
| **Parameters** | ||
| - `fn` (function) |
There was a problem hiding this comment.
Should we document a return value (whatever the function returns?). I mentioned that at the last meeting and #1051 (comment)
I don't mind not returning anything, but then someone always needs to do something like:
await new Promise(resolve => {
browser.test.runWithUserGesture(
() => resolve(browser.permissions.request(perms))
);
});Opposed to:
await browser.test.runWithUserGesture(
() => browser.permissions.request(perms)
);FWIW, our (differently named) test helper in Firefox does currently NOT return the value, to discourage the reader from thinking (incorrectly) that returning a promise would extend the user gesture beyond the current run of the event loop.
There was a problem hiding this comment.
Added a returns section to this method. Chrome usually combines it internally with chrome.test.succeed internally so that's why I omitted it, but I agree it's more ergonomic to the function's result to not have to wrap it in a promise each time. The refactor and impl would be pretty simple to implement in Chrome.
There was a problem hiding this comment.
We return the result in WebKit
Co-authored-by: Rob Wu <rob@robwu.nl>
There was a problem hiding this comment.
Looks good to me. I opened a bug in Firefox to track the implementation of the proposed amendments: https://bugzilla.mozilla.org/show_bug.cgi?id=2073186
There is still an open thread on what succeed/fail should do, I'll let @kiaraarose or @xeenon and you finalize the discussion on that part.
4 participants
No description provided.