w3c / csswg-drafts

CSS Working Group Editor Drafts
https://drafts.csswg.org/
Other
4.5k stars 661 forks source link

Test Metadata #1730

Closed frivoal closed 6 years ago

frivoal commented 7 years ago

The CSSWG used to require at least onerel=help in test metadata to associate each test with a spec (and preferably a section of a spec).

As part of the switch to wpt, rel=help was made optional. My understanding was that the reasoning was:

However, while wpt has a general practice of using paths that match spec names, it is not a strict requirement: the path does not include the spec level (which caused a warning from the build system when checking rel=help, and the GitHub PR bot to post an error comment), though the CSS tests do as legacy from how the merge was done, and tests are generally expected to not be moved (which causes problems with versioned directories if a feature is moved to a different level of the spec) as wpt has a general aversion to moving files due to test runners (e.g., those of vendors) having out of band data (such as result expectations (i.e., PASS/FAIL/CRASH)) that are tied to file paths.

This means that while rel=help when present can be expected to point to the right place, it cannot be expected to be present, its presence should not be a requirement before accepting Pull Requests, and the path may, but does not have to, point to strictly the right place.

Questions: 1) As a WG, are we OK with this status quo? 2) If we are not, do we want to change how we store metadata? 3) What do we do (if anything) about the CSS test harness and its ability to find tests for a spec, based on not necessarily sufficiently accurate metadata. 4) Should we create directories with unversioned shortnames (in addition to the versioned ones we have) and put all new tests there?

frivoal commented 7 years ago

5) Should we move all existing tests to top-level unlevelled directories to be consistent with the rest of wpt? (and lose the build system continuing to reasonably work and hence the test harness, as we'd lose the lints we have to keep the build system working)

gsnedders commented 7 years ago

The lack of versioning only becomes an issue when we want to move a spec to PR and we have a next level already in development; a quick skim suggests this is maybe 50% of the time?

From my understanding, the plan was to move steadily over to the tools others are using for implementations reports and move over to following more wpt policies than we are now, and we were doing this by keeping existing versioned directories (from csswg-test) in css/ with a bunch of extra lints to keep the build system more-or-less working, and putting new directories at the top level without any version suffix.

The lack of versioning is rarely problematic when it comes to building implementations reports even when features have been pushed back at CR to a later level, because in the majority of cases features are sufficiently self-contained that you can exclude all tests for that feature by just excluding one directory; in other cases you need to keep a blacklist of tests. (Of course, maintaining <link rel=help> links in tests to point to the newer spec would also be a way of blacklisting stuff, though you run the risk of people writing tests linking to the latest ED regardless of where their feature comes from, but this is a risk with any form of versioning of the tests.)

It's probably worth pointing out the versioning of tests is essentially only useful for getting to REC, and of little value to anyone else (web developers care about interop and what browsers implement, which doesn't always match cleanly to the levels; implementers tend to care about the latest spec, regardless of status). As such, there's only a brief time where it's actually useful to know what tests constitute level n.

I think it bears repeating that there are plenty of other groups who've used test suites in WPT to reach PR (and whence REC), hence basically every issue is something that's happened before.

As for a subset of the questions (I think addressing everything!):

What do we do (if anything) about the CSS test harness and its ability to find tests for a spec, based on not necessarily sufficiently accurate metadata.

The grouping is essentially a feature of the build system which there's broad agreement to get rid of (indeed, I think the only reason we still have it is for the test harness).

There had been some talk about writing some tool to replace it when it comes to this feature (showing per-section tests and thus also being able to be used for the inline test status markers in specs); I don't think there was such clear consensus that the new tool would actually deal with separate versions of specs (but again, such a feature's usefulness is brief and typically you can do blacklisting quite easily).

That said, I'm unaware of anyone having done any work on any such tool (@gregwhitworth wrote something that grouped based on links, but didn't do what the build system did and plot that relative to the table of contents of the spec), or anyone willing to pay anyone to work on it.

As for the other main feature of the CSS test harness, displaying results, we're getting closer and closer to having daily results on http://wpt.fyi/. Note this doesn't handle manual tests (and there's no really sane way to do so, because it's hard to keep them up-to-date with browser changes and test/support file changes, given with any scripting the latter are impossible to detect in general).

Should we create directories with unversioned shortnames (in addition to the versioned ones we have) and put all new tests there?

No, this seems like the worst possible outcome to me: we should either move everything to unversioned directories (i.e., question 5 above), or do the change for all new tests at a level change.

Should we move all existing tests to top-level unlevelled directories to be consistent with the rest of wpt? (and lose the build system continuing to reasonably work and hence the test harness, as we'd lose the lints we have to keep the build system working)

This is my preferred outcome. Having spoken to a couple of people, I think this is probably doable despite wpt's normal policy on not moving files around if we do this carefully (i.e., we do it all at once, with advance notice, and a clear mapping between files), given the perceived benefit of doing so.

gregwhitworth commented 7 years ago

That said, I'm unaware of anyone having done any work on any such tool (@gregwhitworth wrote something that grouped based on links, but didn't do what the build system did and plot that relative to the table of contents of the spec), or anyone willing to pay anyone to work on it.

I actually had a version that would map to TOC, but that limited it too far as some links were deeper than that, additionally that's how I discovered the issue with regard to mismatching from what was in the TOC. Also, this is exactly why I brought up the linking of musts/should during the test discussion at the F2F as I don't think simply mapping to the TOC is the best approach. I would like for every test submitted to have a rel=help and while you state it doesn't help implementers I'm spending quite a bit of time doing this work and I'd really like them to be there. So it shouldn't be mandatory as I'd prefer tests than no tests, but I would definitely prefer them.

Bikeshed already supports wrapping your things with anchors and it will create a unique id for the page, the only thing that I need to add to my tool or bikeshed is a way to tie the practice of spec writing and test pruning together. We did this for normative changes to specs in CR but it would be preferable to have a flag in bikeshed to run and then check the spec for possible test review. @plinss mentioned that this is possible to include in the email once bikeshed has the lookup information to WPT.

That's about it IMO regarding tooling, shall we do a oneday hackathon and get this busted out? @tabatkins for visibility in hopes he'll offer his python-foo?

css-meeting-bot commented 7 years ago

The CSS Working Group just discussed Metadata.

The full IRC log of that discussion <dael> Topic: Metadata
<dael> github topic: https://github.com/w3c/csswg-drafts/issues/1730
<dael> Florian: May be better for fuller attendance, but in brief.
<dael> Florian: While submitting tests recently I found a difference between what I understand the policy on how to write test and what the web platform tests is. I think there's a mis-match and how we decided to relax our metadata and how the tooling is based.
<dael> Florian: I want to confirm if I'm the only one confused or if others have the misunderstanding.
<dael> Florian: I'm not convinced this is useful without more people.
<dael> Bert: We can keep for the next time. Anyone else want to say something today?
<dael> Bert: Okay, we'll wait for more people
css-meeting-bot commented 7 years ago

The Working Group just discussed Test Metadata.

The full IRC log of that discussion <dael> Topic: Test Metadata
<dael> github: https://github.com/w3c/csswg-drafts/issues/1730
<dael> astearns: You were putting tests in that had the rel metadata?
<dael> Florian: I was moving tests from UI3 to UI4 and I fixed the rel and fixed the file path b/c my memory of when we agreed to merge repos rel was agreed to be optional because same infor was in path
<dael> Florian: I niavely updated the path and was told not to do that. Now I understand they prefer when rel is there and hte path doesn't change.
<dael> Florian: They want rel=help and the path to be accurate, but higher preference is path to not change. The preferred way to do this is unversioned spec in the path. I thought hte path must encode location and it's only a should
<dael> Florian: Currently our tooling relies on rel=help, but I thougth we were eventually supposed to update tooling.
<dael> astearns: Also when we imported CSS test to web platoform we didn't follow the spec name folder, it's all in CSS.
<dael> Florian: And leveled spec names.
<dael> astearns: Right. So I think the point where w fully use path as encoding for the test is when we start adding tests for new spec outside of css directory
<dael> Florian: I think there is already a few.
<dael> Florian: Key point for me is to understand if it's just me who misunderstood or if it's the whole group. We can't programatically rely on the path or rel-help being there.
<dael> Florian: If it' sjust me, fine.
<dael> plinss: Clarification. Our tooling depends on rel=help. wpt was planned to have path as encoding and beyond the spec having sub folders. Some structure. THere was a plan to agment our tool to fallback to path, but that's on hold until path name is reliable.
<zcorpan_> Currently top-level css directories: css-backgrounds css-cascade css-font-display css-font-loading css-fonts css-paint-api css-scoping css-timing css-typed-om cssom-view cssom
<dael> flackr: And there's no plan for that. Spec names unversioned are sorta expected to be reliable, but unversioned not and section level will be more of the same. When a piece of spec moves there's resistance to changing the path.
<astearns> s/flackr/florian/
<dael> astearns: And other outside tooling relying on that means we can never rely on path. We need a requirement to have the rel=help links.
<dael> Florian: But that goes against moving outside css directory
<dael> plinss: No. The css directory was there because that was easy for the transition. Our tooling doesn't care about the path at all if there's a rel=help link
<dael> Florian: But a wpt will not make rel=help manditory
<dael> gsnedders: It's manditory in css subdirectory
<dael> Florian: Right.
<dael> Florian: But we're not mandating the subdirectory
<gsnedders> s/manditory/mandatory/
<dael> fantasai: Do we want to transition outside css directory?
<dael> plinss: There are already some. There are some people moved out, there were some outside already. I don't really care where they are.
<dael> Florian: I kidna came into that...on the one end I think we strongly require this and they strongly require this to be option and we apperently had not agreed.
<dael> astearns: I don't think we decided rel=help is mandatory to check in tests. We don't want a fail if it's not there, but we strongly prefer it. As tests get run we're going to add rel=help meta data to those
<dael> fantasai: We decided to make rel=help optional because it would be replaced by file path. If hta'ts not the case having it optional makes no sense. We need to know what's being tested.
<gregwhitworth> Agree with Elika
<dael> Florian: What I was told is that moving thigns alnog the rec track was a known goal for the wpt. Though it's better to have meta data it still works as a test and it's better to have tihs than no tests.
<dael> fantasai: We agreed to have our tests in their repo, but we shouldn't drop our goals.
<dbaron> I agree with the position -- having regression tests is useful even if they're not used for advancing on the REC track.
<dael> astearns: While our tooling isn't going to use anything in the path, we are still able to rely on top level css folder or top levels that corrispond to our tests to see it is css and if it's lacking rel it can be fixed.
<fantasai> dbaron, sure, but I expect it's going to be used as an excuse for people to be sloppy and putting more work on us
<fantasai> dbaron, it's way easier for the test writer to associate at test with a spec than for us to go back and do so retroactively
<dael> astearns: I'd prefer we keep it optional in that ntohing will stop someone from checking is a test but still strongly preferring it so that we have the practise of checking in the rel=help
<fantasai> dbaron, also people should COMMENT THEIR CODE, and that includes tests, and the WPT folks insisting that tests be uncommented regression dumps while also believing that function declarations should have some kind of commenting is hypocritical
<dael> gsnedders: If I can summerize, two parts. 1 there was some plan to add a per directory file to preserve the mapping to specs even if things got moved around. The other thing was I thought there was some agreement to move away to the tooling people were using for impl tests and wpts
<fantasai> dbaron, tests are maintained code, too
<dael> plinss: We have to rely on the tooling we have until there's a tool that meets our needs.
<dael> Florian: And I don't think there's a preception that we require our meta data b/c w3c, but becuase it makes our lives easier. I've been told we're just imaging requirements from w3c when they're not.
<dael> plinss: We've never done our testing infestructure b/c a w3c mandate. They only mandate we have tests. How we do that is up to us.
<gregwhitworth> s/imaging/imagining
<dael> astearns: fwiw there was smoeone complaining on whatwg irc about an undocumented test.
<dael> fantasai: This is a general problem. There are people that think web platform is a regression dump, but there are other people that have to go back and maintain that code. We're those people We have to look at the code.
<dbaron> fantasai, There are also regression tests where it's not clear what the specs say, but we actually do have interop and should keep it.
<dael> astearns: I don't have a good idea of how many tests we have in wpt that are css tests and lack rel metadata. Does anyone?
<dael> gsnedders: In principle everything in css subdirectory does. IT's a q of howmany are outside.
<dael> astearns: And how many have meta data
<dael> gsnedders: Few outside the subdirectory have it, I think. This is my impression, I don't have numbers.
<dael> astearns: Does the current tooling require rel to check in to css ubdirectory?
<dael> gsnedders: It does
<dael> astearns: So it won't spread in css subdirectory. And maybe we can turn it on elsewhere.
<fantasai> dbaron, sure, and those should be filed as bugs against the specs, preferably with a link to the test in question so that we can update it as necessary
<dael> Florian: But ther'es a plan to move out of subdirectoyr.
<dael> astearns: As we spread out we can spread the requirements to the directorys wil our tests.
<dael> Florian: gsnedders do you think it's welcome?
<dael> gsnedders: I guess not.
<dael> tantek: What's the difference between sub level and top level that's not ours?
<dael> gsnedders: The status quo is we have this one top level with a bunch of different requirements to everything else. I think it's the only one that has any different actual check in level req.
<gsnedders> s/tantek/gregwhitworth /
<dael> Florian: Based on the discussions I've had the rest outside csswg is for the web community and if they want to write tests they shouldn't do our sub directory.
<Florian> s/shouldn't do our sub directory./shouldn't have to follow our unusual rules/
<dael> astearns: I would guess the policy for a top level should be dictated byt he owners. THe cssom tests are outside the directory. so Simon are the tests there using rel links? And would you welcome having that req for those folders?
<dael> zcorpan_: They are currently outside CSS. I believe they mostly have meta data
<fantasai> How are people supposed to cooperate on test coverage if everyone's writing tests in their own little corner and not looking at what else is in the repo? Part of the point here is not to duplicate work by having each implementer write its own version of the same exact test.
<dael> zcorpan_: Exception is prob there are test using like .window.js
<dael> zcorpan_: I ususally at least make sure the path [missed]
<dael> astearns: That's part of your review to make sure the rel links are there?
<bkardell_> present + bkardell_ (but late)
<zcorpan_> I don't know how many of the tests have metadata, but the tests were in css/ before so presumably most have metadata
<dael> astearns: Okay.
<zcorpan_> Yes
<dael> gsnedders: I think some of the cssom tests were already in wpt before the merge
<dael> gsnedders: Therefor those might not have the meta data. That's why they live outside css.
<dbaron> OK, I guess I'm fine with requiring rel=help
<zcorpan_> When I write tests I try to make sure either there's a rel-help or that the directory structure matches which spec is tested
<dael> fantasai: I think it's fine if cssom since they're different. IF we have a sep top level for every spec we'd have some test outside css subdirectory and some within so it makes it hard to find things. Also that's a lot of subdirectories so people finding what they're looking for is hard.
<Florian> q+
<zcorpan_> https://github.com/w3c/web-platform-tests/blob/master/html/README.md is the policy for html/
<dael> astearns: I agree it's messy for some ina nd some out, but it's the current reality. We could not add more to it but adding new tests in css. IT sounds to me like we want to continue to require rel meta data links on all tests inside css directory going forward. Doesn't seem like we can change that and it's helpful to have
<dael> astearns: We can look into requiring the rel link in other folders for tests outside css folder
<astearns> ack Florian
<fantasai> There are less than 200 non-CSSOM tests outside the css/
<dael> Florian: Questions. 1) if it annoys people when files move we should stop that so when we get close to rec and an at risk feature moves moving them is not liked. unversioned folders solves that. Should we start doing that? If yes, when?
<zcorpan_> (I think for Chromium there's no problem with moving tests, the tooling should figure that out. Might be different for other vendors.)
<gsnedders> fantasai: including almost all of Houdini, IIRC
<Florian> q+
<gsnedders> zcorpan_: (expectation data hardcodes the path)
<dael> astearns: Any thoughts on moving to non-versioned folders?
<dael> fasi"m okay with that.
<dael> s/fasi"m/ fantasai: I'm
<dael> Florian: If we do it in one shot and warn people it might be okay
<zcorpan_> gsnedders: (I was informed that the downstream updater figures out expectations for moved tests.)
<dael> gsnedders: There's some consensus it's okay to do these thigns in big moves to get rid of cases where css uses policies not used byt he rest fof the repo
<dael> Florian: THat's pref? moving css/cssfoo3 or move everything to css/foo all at once?
<dael> gsnedders: There's a part of me in favor of keeping status quo until we have different tooling and them move to top level at once.
<dael> fantasai: If we want to move to havin the path encode data we should do that at same time. But if we drop versions we can't do subscriptiosn reliably because they'll change level to level. Some will be same, some won't.
<dael> fantasai: I think it'll be tricky to have unvision module directorys but also do waht to have per section sub directories. We can do one or the other but not both.
<dael> Florian: I think we might want subdirectories per topic, but if we can't move the shouldn't be fine grained.
<dael> fantasai: Agree. We should just have per module directories on loevel. It would be a good idea to move the tests outside css subdirectory in and then maintin that for all tests. If we put them top level it'll be 60 top level and be hard for people to find all css tests
<dael> fantasai: I think it makes the repo more usable if we don't have all 60+ modules scattered across the top level
<dael> astearns: gsnedders can I give you an action to look at moving to unversioned folders in one go to avoid file movemenets in the futuer? Within the css subdirectory?
<dael> gsnedders: I guess
<dael> Florian: Until we have the answer, should we start making unversioned subdirectory to avoid making it worse for new tests?
<dael> astearns: I think it makes sense for new tests
<dael> fantasai: If we're going to move a bunch people will have to deal. IF we don't move we'll have to move those things back.
<dael> astearns: Perhaps new test suite if you have to make a new folder, make it unversioned.
<astearns> ack Florian
<dael> Florian: Question 2) If we move out of css subdirectory for purpose of no logner following css rules, what do we lost beyond rel=help?
<dael> gsnedders: I can't remember all. There's the check for unique files names within test suite. We have rel=help. There's another I can't remember
<dael> Florian: These are things we only care about b/c build tool relies on them. Correct?
<dael> gsnedders: Yes. All CSS only events are there to keep the build system working.
<zcorpan_> css/ also can't use .window.js, .worker.js, .any.js, since they can't encode rel=help
<zcorpan_> though adding ability to encode a spec link in those seems possible
<dael> plinss: Beyond that the build system org tests into per spec suites and generates other versions of tests. OTher tooling relies on that outpul like test harness and spec annotations. Also if we ever want anything to say what tests a spec change may break. So I keep hearing we might move, but if we lose the meta data we lose these extra capabilities.
<gsnedders> zcorpan_: also that doesn't work in the build system, because they don't use wptserve
<dael> Florian: This idea is that other people get by without this and why can't we. I don't know why we should change.
<dael> plinss: We care about taking specs to rec. It's a non goal of wpt so do not expect support for that goal.
<dael> astearns: Other people also require a test when you change aspec.
frivoal commented 6 years ago

Closing, this discussion has lived its time and achieved its purpose. Other test discussions are open, so we can continue there.