[Connection Allowlist] Stop waiting in reporting tests. [chromium/src : main]
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
WDYT about this potential improvement to some tests, Antonio?
Related details
- Antonio Sartori
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
async function pollReportsUntil(endpoint, id, condition, timeout = 3000,
interval = 100) {(nit) reusing `t.step_wait_func` is probably preferred, also because it takes into account the timeout multiplier
cf. https://web-platform-tests.org/writing-tests/testharness-api.html#Test.step_wait_func
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
async function pollReportsUntil(endpoint, id, condition, timeout = 3000,
interval = 100) {(nit) reusing `t.step_wait_func` is probably preferred, also because it takes into account the timeout multiplier
cf. https://web-platform-tests.org/writing-tests/testharness-api.html#Test.step_wait_func
Hrm. New to me! :)
Do you have thoughts about how you'd structure this? That is, I guess we could pass the test object in here and use `t.step_wait_func` as part of this function? Or do you expect the callsites to wrap this method call in that helper?
Related details
- Antonio Sartori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
async function pollReportsUntil(endpoint, id, condition, timeout = 3000,
interval = 100) {Mike West(nit) reusing `t.step_wait_func` is probably preferred, also because it takes into account the timeout multiplier
cf. https://web-platform-tests.org/writing-tests/testharness-api.html#Test.step_wait_func
Hrm. New to me! :)
Do you have thoughts about how you'd structure this? That is, I guess we could pass the test object in here and use `t.step_wait_func` as part of this function? Or do you expect the callsites to wrap this method call in that helper?
yes, I was thinking about something like the following.
There is a change in behavior though, because this will throw, instead of returning the reports, if the timeout expires. I guess it's still fine for your purpose. If not, let's stick to your code.
```
async function pollReportsUntil(t, endpoint, id, condition, timeout = 3000,
interval = 100) {
return t.step_wait(
async function() {
const reports = await pollReports(endpoint, id);
if (condition(reports)) {
return reports;
}
},
"Deadline exceeded and no reports received.",
timeout,
interval
});
}
```
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
Unfortunately, `step_wait` (and `step_wait_func` that it depends upon) don't return the value. They resolve a promise if the value returned is truthy. So `return reports` doesn't hand the set of reports back to the caller, but just resolves a promise with `undefined`. (See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/wasm/core/js/harness/testharness.js;drc=85c57e0ffa499cdab1105f2336722f474ce9457c;l=3123 and https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/wasm/core/js/harness/testharness.js;drc=85c57e0ffa499cdab1105f2336722f474ce9457c;l=3032. I need the results, as we have tests verifying allowed requests (which shouldn't generate reports) and blocked requests (which should), so I need the actual value `pollReports()` eventually produces.
I dug around a bit more in testharness, but I didn't find anything that would help. Given that, I think the code currently here is better than status quo, but I do agree it would be better to use the timeout multiplier... it doesn't look like that's exposed outside the `Tests` object in testharness, which I don't think I have access to. :/
Related details
- Antonio Sartori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
1 comment
async function pollReportsUntil(endpoint, id, condition, timeout = 3000,
interval = 100) {Mike West(nit) reusing `t.step_wait_func` is probably preferred, also because it takes into account the timeout multiplier
cf. https://web-platform-tests.org/writing-tests/testharness-api.html#Test.step_wait_func
Antonio SartoriHrm. New to me! :)
Do you have thoughts about how you'd structure this? That is, I guess we could pass the test object in here and use `t.step_wait_func` as part of this function? Or do you expect the callsites to wrap this method call in that helper?
Mike Westyes, I was thinking about something like the following.
There is a change in behavior though, because this will throw, instead of returning the reports, if the timeout expires. I guess it's still fine for your purpose. If not, let's stick to your code.
```
async function pollReportsUntil(t, endpoint, id, condition, timeout = 3000,
interval = 100) {
return t.step_wait(
async function() {
const reports = await pollReports(endpoint, id);
if (condition(reports)) {
return reports;
}
},
"Deadline exceeded and no reports received.",
timeout,
interval
});
}
```
Unfortunately, `step_wait` (and `step_wait_func` that it depends upon) don't return the value. They resolve a promise if the value returned is truthy. So `return reports` doesn't hand the set of reports back to the caller, but just resolves a promise with `undefined`. (See https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/wasm/core/js/harness/testharness.js;drc=85c57e0ffa499cdab1105f2336722f474ce9457c;l=3123 and https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/wasm/core/js/harness/testharness.js;drc=85c57e0ffa499cdab1105f2336722f474ce9457c;l=3032. I need the results, as we have tests verifying allowed requests (which shouldn't generate reports) and blocked requests (which should), so I need the actual value `pollReports()` eventually produces.
I dug around a bit more in testharness, but I didn't find anything that would help. Given that, I think the code currently here is better than status quo, but I do agree it would be better to use the timeout multiplier... it doesn't look like that's exposed outside the `Tests` object in testharness, which I don't think I have access to. :/
Uh, sad... but ok. Sounds good. Sorry for the extra work I generated...
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |