search

web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

testharness.js: Replace assert_throws and promise_rejects with more fine-grained APIs #29

Closed Ms2ger closed 4 years ago

jgraham commented 4 years ago

This seems reasonable to me, but I'd like to check that e.g. @bzbarsky and @annevk are on board with the proposed changes.

bzbarsky commented 4 years ago

I'm very much on board with this. So much so that I filed the issue requesting this change (https://github.com/web-platform-tests/wpt/issues/11726) and wrote the testharness changes to implement it (https://github.com/web-platform-tests/wpt/pull/19054).

jgraham commented 4 years ago

I'm very much on board with this. So much so that I filed the issue requesting this change and wrote the testharness changes to implement it

Oh, hah I missed that bit of context.

Unrelated to that, in the risk section of the RFC it says:

Updates to testharness.js haven't always been made together with the test updates in all browser engine repos.

We make those updates atomically in gecko and servo, so from our point of view no additional coordination is required to land this kind of change.

bzbarsky commented 4 years ago

So what is the next step here? Are we waiting for someone specific to look things over or make a call?

jgraham commented 4 years ago

The process is that we allow a week for objections.

foolip commented 4 years ago

@youennf can you check how this would impact WebKit?

jgraham commented 4 years ago

I think this has been a week and there aren't substantive objections.

I think it's reasonable to land this with same_value and consider moving to Object.is as a followup; traditionally we've been slower to adopt new ES features in testharness.js since it's occasionally used to test relatively outdated browsers; I don't know if this makes the cut or not, but let's not block this issue on that discussion.

foolip commented 4 years ago

@Ms2ger @bzbarsky what's the status of implementing this RFC? Will we need to audit use of assert_throws and promise_rejects in Chromium soon?

cc @stephenmcgruer

bzbarsky commented 4 years ago

The new functions are landed. New tests should use them. I'm not sure there's any specific plan or timeline for converting the existing shared-test uses so far.

bzbarsky commented 4 years ago

That said, there are ~4300 assert_throws uses in wpt so far. Of those, I think ~2100 are very easy to rewrite automatically to assert_throws_js without even messing up indentation and another ~72 are auto-rewritable with some indentation messup. I'm going to give that a show and see how things look....

Another ~1400 should be auto-rewritable to assert_throws_dom, I think.

The remaining ones, (~700 for those keeping count), I guess we see what they look like.

bzbarsky commented 4 years ago

I'm going to take a stab at the auto-rewriting and fixing the Gecko bugs it exposes... it was on my list anyway.

foolip commented 4 years ago

Thanks @bzbarsky! When the time to remove assert_throws and promise_rejects is closer, can you update this thread?

stephenmcgruer commented 4 years ago

Unless I'm counting wrong, the number of promise_rejects has fallen substantially in Chromium from the 300 reported above.

["assert_throws("](https://cs.chromium.org/search/?q=%22assert_throws(%22+file:third_party/blink/web_tests+-file:external/wpt&sq=package:chromium&type=cs ) - ~300

["promise_rejects("](https://cs.chromium.org/search/?q=%22promise_rejects(%22+file:third_party/blink/web_tests+-file:external/wpt&sq=package:chromium&type=cs ) - ~70

bzbarsky commented 4 years ago

@foolip will do! I haven't even looked at promise_rejects yet. ;)

bzbarsky commented 4 years ago

Fwiw, it would also help if people stopped adding new assert_throws. Just in the last few days, since I wrote some patches to remove a bunch, three changesets have added more...

youennf commented 4 years ago

@youennf can you check how this would impact WebKit?

We have some WebKit internal tests using assert_throws/promise_rejects. Something like 50 files at least. I guess the plan is to keep assert_throws/promise_rejects support in testharness.js for a while but make the linter forbid its use?

foolip commented 4 years ago

Adding a lint for it would cause people to notice that there are new forms that they should use instead. But there are still over 1000 files matching assert_throws[^_] so maybe it would be a good idea to get that down to a smaller number first.

zcorpan commented 4 years ago

What can be done sooner is to update the documentation. https://github.com/web-platform-tests/wpt/issues/21288

jgraham commented 4 years ago

I'm not sure what the specific disadvantage of having a lint with a lot of existing cases is; maybe some performance penalty. In any case I created a PR: https://github.com/web-platform-tests/wpt/pull/21294

stephenmcgruer commented 4 years ago

I agree that it's reasonable to land a lint step, but looking at the PR I think we should have some documentation to link people to before landing it. assert_throws_js and assert_throws_dom are tricky to understand at face value, and currently there's nothing in https://web-platform-tests.org/ to explain to me (as a test writer) which I should be using.

bzbarsky commented 4 years ago

Fwiw, I have patches to get the number of assert_throws in the test suite down to ~200. Hoping to get those up for review today or so.

jgraham commented 4 years ago

I added some basic docs changes to my PR.

bzbarsky commented 4 years ago

I created https://github.com/web-platform-tests/wpt/pull/21350 for the main mass-replacements of assert_throws.

stephenmcgruer commented 4 years ago

With bzbarsky's recent changes, we're down to 121 counts of assert_throws( in WPT.

I have been fielding some light push-back from folks as these changes filter through. Some folks are finding them confusing in terms of which function to use, what they actually mean, etc. I don't think any action is needed yet, but this is on my mind as I look at assert_object_equals too.

bzbarsky commented 4 years ago

With bzbarsky's recent changes, we're down to 121 counts of assert_throws( in WPT.

Hmm. It's 51 in my local tree on master, as far as I can tell. And 41 once https://github.com/web-platform-tests/wpt/pull/21388 merges, I think.

Some folks are finding them confusing in terms of which function to use, what they actually mean, etc.

Do you have a sense about whether the documentation for the functions in testharness.js is unclear or whether they had not seen those docs and only had the function names to go on?

stephenmcgruer commented 4 years ago

Hmm. It's 51 in my local tree on master, as far as I can tell. And 41 once web-platform-tests/wpt#21388 merges, I think.

Ah, I concur. I had included .git by mistake in my naive grep. 51 once that is excluded.

Do you have a sense about whether the documentation for the functions in testharness.js is unclear or whether they had not seen those docs and only had the function names to go on?

So I would guess nobody reads the source. We hope they might read https://web-platform-tests.org/, but that doesn't yet have documentation. That said, no, I'm afraid I don't have a good sense of whether it would make sense to them if they had read the documentation or not (sorry, not very useful :()

bzbarsky commented 4 years ago

I posted https://github.com/web-platform-tests/wpt/pull/21582 to remove a bunch of the promise_rejects calls with promise_rejects_js. Once that lands, I have similar changes for promise_rejects_dom.

Still working on mopping up the remaining assert_throws bits too.

bzbarsky commented 4 years ago

OK, at this point I have all of assert_throws and promise_rejects removed locally. Just waiting on various open PRs to merge so I can put up the final cleanup PRs...

bzbarsky commented 4 years ago

This is now completely done.

stephenmcgruer commented 4 years ago

FYI as of https://crbug.com/1051932#c22 this is now (finally!) done in Chromium downstream too.