Skip to content

Conversation

@domenic
Copy link
Member

@domenic domenic commented Sep 27, 2015

This adds a new directory, to-upstream, which has a parallel hierarchy to the web platform test suite. All test files in it will be run.

It demonstrates this process by porting some tests from our attributes tests.

Review please @Sebmaster @Joris-van-der-Wel. I'll probably port more over time to this, and work on figuring out how to get them running in the browser. Although actually that should probably wait until @Joris-van-der-Wel fixes our base test runner situation...

@domenic domenic force-pushed the wpt-adapter branch 3 times, most recently from 912d943 to b5ab11f Compare September 27, 2015 16:24
@Joris-van-der-Wel
Copy link
Member

Good idea.

I do not see anything funky if i read over it.

Have you found any test case which was already covered by an existing WPT?

Is there any detailed style guide for writing javascript WPT?. I have read testthewebforward.org but when it comes to the javascript tests, it only discusses the API. I am asking because I've seen a few javascript tests that were incredibly complicated, more complicated than the implementation that they are actually testing. Are such tests just a relic of the past, or is this really part of "the" style? For example https://github.com/w3c/web-platform-tests/blob/master/dom/traversal/NodeIterator.html was almost useless for me when I was implementing NodeIterator.

@domenic
Copy link
Member Author

domenic commented Sep 27, 2015

Have you found any test case which was already covered by an existing WPT?

I think some of them definitely are in spirit. (Although, currently we cannot enable the big attributes.html test, due to us still having Attr inherit from Node.)

But, the exact details of the test will differ a little, not just in trivial things like attribute names, but also in sequencing of operations and so on.

Is there any detailed style guide for writing javascript WPT?

Yeah, I only found the testthewebforward.org stuff too. I think there's a couple factors in the complexity. One, some of the more prominent contributors do seem to really enjoy generative testing, and don't adhere to the philosophy that tests should be so simple you can't get them wrong. Two, there are a lot of older tests as well, imported from browsers or elsewhere.

But I think we have a pretty good sense of what a good test case is in jsdom, in terms of simplicity etc. So we should just stick to our sensibilities. The only thing I really worry about is if they don't like our file layout choices, e.g. maybe everything in this PR is supposed to go in attributes.html. But it's probably fine.

@jgraham
Copy link

jgraham commented Sep 27, 2015

wpt doesn't have a style guide as-such because it accepts contributions from a diverse range of sources and it's more important that we get tests than that every file makes exactly the same choices on use of whitespace. That said there is a lint that must pass before tests can be accepted that checks for some simple style issues like trailing whitespace and use of unix-style line endings.

One consideration that we do have is performance; loading 1000 tests in individual files in a browser is much slower than loading one file that contains the same 1000 tests. So there is a slight preference for combining multiple tests into one file (but still as separate test()s) because when you run these things hundreds of times a day performance matters.

As for the tangent, before you dismiss the more complex tests, it's worth looking at how effective they are at finding bugs in browsers. Often people hand writing tests for a feature will cover one or two simple cases and then maybe one or two edge cases and then declare that they are done. Generating tests from data gives you a lot more coverage for the same effort, although there are of course some tradeoffs.

@Joris-van-der-Wel
Copy link
Member

I have nothing against combining tests into one file for performance or organizing. For javascript tests, I see a file more as a "suite", instead of a single test.

As for generative tests: My issue just is that some of the ones that I have encountered were difficult to decipher. A while back I was implementing a new feature and one of the generative tests was failing. It took me a while to discover that it was a specification bug in DOM whatwg/dom#24 whatwg/dom#27. I had difficulty recreating the state the generative test was in at the time of failure. This particular part of the spec was only tested by a generative test.

I am not opposed to generative tests/fuzzers by default, I just believe some of them could have been written with more clarity or debug-ability. The test that i was debugging is using eval, has 4 nested loops, uses no functions, and the failure messages describe the state of the test method itself, instead of something relevant to the feature being tested.

@jgraham
Copy link

jgraham commented Sep 28, 2015

The way we model multiple tests in one file (as least as far as running wpt at Mozilla goes) is "test" and "subtest". Every file is a test, which can have a status like timeout, error, crash or ok (ok just being the status indicating that there was no other status; not a status indicating that all tests passed). Subtests are individual *test() calls which hae statuses like pass, fail, error or timeout. The concept of "suite" doesn't really exist.

As for the generative tests; I think we are in agreement. They are useful but could be easier to debug. I hope you understand why given the choice between having the coverage and having something easy to debug we took the coverage. Maybe for future tests we can think of ways to make the internal state of the test easier to recreate.

(FWIW the thing that really annoys me is people that submit quite straightforward tests wrapped in layers of test-specific abstraction that doesn't do much, but that every new person reading the tests has to learn).

This adds a new directory, to-upstream, which has a parallel hierarchy to the web platform test suite. All test files in it will be run.

It demonstrates this process by porting some tests from our attributes tests.
@domenic domenic merged commit 9022c42 into master Oct 2, 2015
@domenic domenic deleted the wpt-adapter branch October 2, 2015 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants