Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RFC for `crash` test type #33

Merged
merged 2 commits into from Nov 11, 2019

Conversation

@jgraham
Copy link
Contributor

jgraham commented Oct 9, 2019

Add a test type that just ensures that a page can be loaded without error.

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Oct 9, 2019

@emilio I think you were interested in this. If you have feedback it would be great, particularly if you have evidence that sharing load-type tests (e.g. gecko crashtests) between engines is effective at finding issues.

@foolip
foolip approved these changes Oct 9, 2019
@zcorpan
zcorpan approved these changes Oct 9, 2019
Copy link
Member

zcorpan left a comment

wpt already has a bunch of crash tests, that might use testharness.js or reftest just to be able to run. e.g.
css/css-overflow/shrink-to-fit-auto-overflow-relayout-crash.html
css/css-pseudo/first-letter-of-html-root-crash.html

rfcs/load_test.md Outdated Show resolved Hide resolved
rfcs/load_test.md Outdated Show resolved Hide resolved
@jugglinmike

This comment has been minimized.

Copy link
Contributor

jugglinmike commented Oct 12, 2019

Crashes and other issues caught by these tests may not be sufficiently common between browsers to make sharing these tests a good use of resources.

This was my concern while reading the proposal, so I'm glad you included it! Personally, I'm pessimistic that tests like these do provide much value between implementations. I'm no implementer, though, so I'm very curious about what @emilio has to say on the subject.

@emilio

This comment has been minimized.

Copy link

emilio commented Oct 12, 2019

And this was a very simple test runner, there's tons of test-cases that I haven't ran or what not.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Oct 14, 2019

I'd rather call them crash tests, given that's what they're called everywhere else.

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Oct 14, 2019

The reason for not calingthem crashtests was that crashing is only one possible failure mode; they could also fail with an assert or sanitizer failure or similar in vendor infrastructure. But I'm sympathetic to the idea that "load" can be confusing and there is precedent here.

@jgraham jgraham force-pushed the load_test branch from 1a7fcec to e9e4ab5 Oct 14, 2019
@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Oct 14, 2019

I think that's an overly pedantic viewpoint to take when there's prior art in all vendors here already, even if the name is slightly confusing. (Just make all assertions and saniziter failures crashes, pff!)

@Ms2ger
Ms2ger approved these changes Oct 14, 2019
rfcs/load_test.md Show resolved Hide resolved
rfcs/load_test.md Show resolved Hide resolved
rfcs/load_test.md Outdated Show resolved Hide resolved
rfcs/load_test.md Show resolved Hide resolved
@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Oct 31, 2019

web-platform-tests/wpt#20017 is a draft PR for this RFC. It's not done yet (hence draft), but what's there seems to work.

Copy link
Contributor Author

jgraham left a comment

I think the only substantive decision left here is whether to use -crash or -crashtest in the filename. The former requires some additional work to rename existing files, but is also shorter and closer to existing practice for WebKit. I'd like to make a decision there and then close out this issue so we can land the changes.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Nov 5, 2019

My preference is for -crash.

@jgraham jgraham changed the title Add RFC for `load` test type Add RFC for `crash` test type Nov 11, 2019
@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Nov 11, 2019

OK changed to -crash and crashtests. So I don't think there are more remaining issues and will merge.

@jgraham jgraham merged commit 4940834 into master Nov 11, 2019
@foolip foolip deleted the load_test branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.