[Extensions] Overload chrome.test.assertThrows to support w3c proposal [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
1 comment
Hi Tim! Doing assertThrows() now 😄. We can consider the tests to have passed since they passed on previous patchsets and I made some minor comment changes only for this recent upload.
Related details
- Tim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tim (Gerrit)
Tim added 1 comment![]( "Open in Gerrit")
Hmmm, this way of using choices and repeating different arguments in different positions seems like quite a hack from its normal intended use. I get that you do the custom work to disambiguate which signature you think has been passed in the custom bindings, but it feels bad to me that the schema says `chrome.test.assertThrows(chrome.tabs.create, "foo", "foo", "foo")` would be a valid call.
I'm going to pass this one over to Devlin for now to get their take on the general approach and if they have any alternative ideas on how to do this best.
rdevlin.cronin@: If you think the overall approach here is fine, feel free to pass it back to me to take a closer look.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Hmmm, this way of using choices and repeating different arguments in different positions seems like quite a hack from its normal intended use. I get that you do the custom work to disambiguate which signature you think has been passed in the custom bindings, but it feels bad to me that the schema says `chrome.test.assertThrows(chrome.tabs.create, "foo", "foo", "foo")` would be a valid call.
I'm going to pass this one over to Devlin for now to get their take on the general approach and if they have any alternative ideas on how to do this best.
rdevlin.cronin@: If you think the overall approach here is fine, feel free to pass it back to me to take a closer look.
For extra context Devlin: the design said to move the old signature to assertThrowsSelf and then have the browser.test proposal's assertThrows signature be the default. But it seemed not too complex in practice to just support both so I tried this way.
There isn't much use of assertThrows currently so it wouldn't be too onerous to still do assertThrowsSelf. I could also try to just coerce the current usages to use the browser.test proposal's assertThrows signature so that we only have one definition. LMK which would be more preferred.
Related details
- Devlin Cronin
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. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
Justin LulejianHmmm, this way of using choices and repeating different arguments in different positions seems like quite a hack from its normal intended use. I get that you do the custom work to disambiguate which signature you think has been passed in the custom bindings, but it feels bad to me that the schema says `chrome.test.assertThrows(chrome.tabs.create, "foo", "foo", "foo")` would be a valid call.
I'm going to pass this one over to Devlin for now to get their take on the general approach and if they have any alternative ideas on how to do this best.
rdevlin.cronin@: If you think the overall approach here is fine, feel free to pass it back to me to take a closer look.
For extra context Devlin: the design said to move the old signature to assertThrowsSelf and then have the browser.test proposal's assertThrows signature be the default. But it seemed not too complex in practice to just support both so I tried this way.
There isn't much use of assertThrows currently so it wouldn't be too onerous to still do assertThrowsSelf. I could also try to just coerce the current usages to use the browser.test proposal's assertThrows signature so that we only have one definition. LMK which would be more preferred.
I ended up attempting (in Patchset 12) to just only use the browser.test proposal's version (`browser.test.assertThrows(fn, expectedError, message)`) and updating existing tests to use it. Besides the large diff due to updating the tests, it ends up being a lot simpler.
Devlin: lmk which path you'd prefer us to take and I'll update the commit message as well if necessary.
Related details
- Devlin Cronin
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. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
Justin LulejianHmmm, this way of using choices and repeating different arguments in different positions seems like quite a hack from its normal intended use. I get that you do the custom work to disambiguate which signature you think has been passed in the custom bindings, but it feels bad to me that the schema says `chrome.test.assertThrows(chrome.tabs.create, "foo", "foo", "foo")` would be a valid call.
I'm going to pass this one over to Devlin for now to get their take on the general approach and if they have any alternative ideas on how to do this best.
rdevlin.cronin@: If you think the overall approach here is fine, feel free to pass it back to me to take a closer look.
Justin LulejianFor extra context Devlin: the design said to move the old signature to assertThrowsSelf and then have the browser.test proposal's assertThrows signature be the default. But it seemed not too complex in practice to just support both so I tried this way.
There isn't much use of assertThrows currently so it wouldn't be too onerous to still do assertThrowsSelf. I could also try to just coerce the current usages to use the browser.test proposal's assertThrows signature so that we only have one definition. LMK which would be more preferred.
I ended up attempting (in Patchset 12) to just only use the browser.test proposal's version (`browser.test.assertThrows(fn, expectedError, message)`) and updating existing tests to use it. Besides the large diff due to updating the tests, it ends up being a lot simpler.
Devlin: lmk which path you'd prefer us to take and I'll update the commit message as well if necessary.
(actually the latest patchset, 12 was failing with a couple of tests I missed).
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 3 comments![]( "Open in Gerrit")
Justin LulejianHmmm, this way of using choices and repeating different arguments in different positions seems like quite a hack from its normal intended use. I get that you do the custom work to disambiguate which signature you think has been passed in the custom bindings, but it feels bad to me that the schema says `chrome.test.assertThrows(chrome.tabs.create, "foo", "foo", "foo")` would be a valid call.
I'm going to pass this one over to Devlin for now to get their take on the general approach and if they have any alternative ideas on how to do this best.
rdevlin.cronin@: If you think the overall approach here is fine, feel free to pass it back to me to take a closer look.
Justin LulejianFor extra context Devlin: the design said to move the old signature to assertThrowsSelf and then have the browser.test proposal's assertThrows signature be the default. But it seemed not too complex in practice to just support both so I tried this way.
There isn't much use of assertThrows currently so it wouldn't be too onerous to still do assertThrowsSelf. I could also try to just coerce the current usages to use the browser.test proposal's assertThrows signature so that we only have one definition. LMK which would be more preferred.
Justin LulejianI ended up attempting (in Patchset 12) to just only use the browser.test proposal's version (`browser.test.assertThrows(fn, expectedError, message)`) and updating existing tests to use it. Besides the large diff due to updating the tests, it ends up being a lot simpler.
Devlin: lmk which path you'd prefer us to take and I'll update the commit message as well if necessary.
(actually the latest patchset, 12 was failing with a couple of tests I missed).
Thanks for the details!
Given there aren't *too* many callsites here to update, I'd prefer we just update them all to use the new version (as appears to have been done here).
Thanks, Justin!
Just replying here and passing it back to Tim for the full review.
{'origins': []}, {'cookies': true}),for many of these, IMO, it's slightly cleaner to use bind() rather than wrapping in a function call:
```
chrome.browsingData.remove.bind(null, {'origins': []}, {'cookies': true})
```
(I think it's more obvious what function is expected to throw and it cleans up the stack trace a little, since there isn't an anonymous wrapper function in the trace)
But I don't feel super strongly. Feel free to use your best judgment.
Related details
- Justin Lulejian
- Tim
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. |