Skip to content

Conversation

@jakearchibald
Copy link
Contributor

@svgeesus
Copy link
Contributor

It would be useful to add a rel=help pointing to the actual spec section being tested, as documented here
https://web-platform-tests.org/writing-tests/css-metadata.html

@tabatkins
Copy link
Contributor

This is a really weird way to structure these tests, imo - you're putting all the actual testing logic together into the script, and then just using the separate test files to make each testing function reftest-able.

It's much better for each test to stand on its own as much as possible; shared utility functions in a separate script are fine, but it should be relatively easy to read the test file itself and understand what's being tested and how.

Also, yes, each test should have a <link rel=help> (pointing to the section of the spec it's testing; use multiple if necessary) and <meta name=author> (so we know who to contact if we have questions later). (The practice of several Googlers to just put "Google Inc" is awful and we need to stop.)

@jakearchibald
Copy link
Contributor Author

This is a really weird way to structure these tests, imo - you're putting all the actual testing logic together into the script, and then just using the separate test files to make each testing function reftest-able.

It's much better for each test to stand on its own as much as possible; shared utility functions in a separate script are fine, but it should be relatively easy to read the test file itself and understand what's being tested and how.

My goal here is to ensure that the same things are being tested for mix-blend-mode in HTML and SVG, and background-blend-mode. It seems bad that test-cases across the three would diverge, so I made all three use the same array of tests.

Is this a goal worth pursuing, or is that kind of consistency unimportant to CSS tests?

I could move the generation of the elements to the various HTML files. This might create some duplication between the main file and the ref file, but it'd ensure they're all using the same test data. Or, is the creation of elements with JS a no-go?

Also, yes, each test should have a <link rel=help> (pointing to the section of the spec it's testing; use multiple if necessary)

How does that work at this stage? In other groups, I've worked on spec changes and tests in tandem, and both are landed around the same time. I can't point to a spec right now because it's in a PR. It feels like a deadlock:

  • I can't complete the review of this PR without <link rel=help> pointing to a spec.
  • Landing the spec without tests seems bad.

What's the right way to resolve this?

@tabatkins
Copy link
Contributor

My goal here is to ensure that the same things are being tested for mix-blend-mode in HTML and SVG, and background-blend-mode. It seems bad that test-cases across the three would diverge, so I made all three use the same array of tests.

Is this a goal worth pursuing, or is that kind of consistency unimportant to CSS tests?

If you feel the need to centralize the test data in the module and import it for each file, that's fine, but I also think it's perfectly fine to just duplicate it across the tests so it's readable at each source (possibly with a comment on each telling people to edit them in tandem). We don't change tests very often, but we read them a lot, so it's good to optimize for the latter imo.

I could move the generation of the elements to the various HTML files. This might create some duplication between the main file and the ref file, but it'd ensure they're all using the same test data. Or, is the creation of elements with JS a no-go?

Making elements with JS is fine. Duplicating structure between the main and ref file is perfectly fine, as well, if it means both are easier to read and understand. Right now, the test and ref are generated by calling into a single function that just does two almost completely different things based on a flag, so I don't think there's much duplication avoided anyway?

How does that work at this stage? In other groups, I've worked on spec changes and tests in tandem, and both are landed around the same time. I can't point to a spec right now because it's in a PR. It feels like a deadlock:

Ah, I wasn't aware this test was for a PR. You can absolutely still point to the spec; the anchors won't be valid until the PR lands, but that's temporary and much better than (a) having to remember to come back after the PR lands to add the links, or (b) never remembering to do that at all. Just set the tests up for presumed success and you'll be fine. ^_^

@jakearchibald
Copy link
Contributor Author

jakearchibald commented Dec 2, 2021

Ok, I think I'm getting a handle on what to do. I'll lean towards duplication rather than abstraction.

The bit I'd like to keep is the generation of the expected composited value. I think it's good to show the working there, but also, if it turns out that browsers end up rounding differently, then we can fix up the function once.

This means I'll still be generating the elements with JS, but I'll switch to using template strings so it looks more like HTML. Also, the code to generate the elements will be in each HTML file, rather than centralised in one script.

Thanks for explaining all this!

@jakearchibald
Copy link
Contributor Author

@tabatkins is this better?

@tabatkins
Copy link
Contributor

Yeah, looks good. Just needs the author/spec links and we're good. (I haven't actually tried the tests themselves, but I understand what they're doing and assume that they're testing things correctly.)

@jakearchibald
Copy link
Contributor Author

Cool, I'll add those once foks are happy with the spec changes, since I'll know the right anchors by then.

Copy link
Contributor

@tabatkins tabatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ on content; needs spec/author links before merging, but i trust Jake'll add those before hitting the Merge button

@jakearchibald
Copy link
Contributor Author

I'll stake my reputation on it

@jakearchibald
Copy link
Contributor Author

@tabatkins the spec change has landed, so this is good to go!

@tabatkins tabatkins merged commit 2f7da96 into master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants