Re: Repository layout

On May 31, 2012, at 6:53 AM, Robin Berjon wrote:

> Hi James,
> 
> thanks for bringing this up, it so happens that just as you were writing this email Dom and I were discussing the exact same problem.
> 
> On May 30, 2012, at 13:53 , James Graham wrote:
>> For many test repositories we are using a submitted/approved directory structure. This is not working well for several reasons:
>> 
>> * There are typically far more useful tests in the submitted directories than the approved directories. This is due to a general lack of time/interest in reviewing tests (I am just as guilty as anyone). I doubt this situation will change.
>> 
>> * It makes it difficult for us to import the tests. Because of the way we test it is very helpful if the paths to tests remain constant (cool URIs and all that). Moving the tests around is a severe inconvenience as we have to update our metadata with the new paths.
> 
> Additionally, it is causing regular problems with the Test Framework tool. Currently, submitted test suites tend to be imported into it so that people can run them, but when tests are moved around the DB entries are not updated either for the approved or for the submitted suites. Changing this will not make this problem entirely go away since it's possible that tests will still be removed, but it ought to make this breakage much rarer.

My guess here is that the manifests are probably not being used properly (though I haven't really looked at the particular problem you're describing). The test case import should be removing tests not found in a newer version of the manifest from the suite, although the tests themselves and their results will be preserved in the DB (by design). This way if the test shows up in a different suite (or the same one later) the old results are still there.

The path should not be part of the test name, so if the test moves it's irrelevant to most of the DB. Only the `testpages` table should be updated with the proper URLs to the tests themselves. The `testcase` field in the `testcases` table should be invariant and not have any path component (that should be in the `testpages` table).  The original was built with the CSS build system output in mind, which is a flat directory per test format, so there's probably some improvements that need to be made in the test case import script for hierarchical test directories there.

My understanding is that most of the suites aren't using the revision information for the tests properly either. This is another field that the build system populates properly when it generates the manifests.

Even for those test suites where a build step isn't necessary, the build tools can still be useful for generating the proper manifest files...

> 
> What's more, submitted tests are inside ./submitted/$submitter/foo.html and approved tests are in ./approved/foo.html — a lot of the time they reference ../../common/some-useful-script.js and break when you move them. It's not a huge deal, but it's a regular annoyance.

FWIW, those kinds of path fixups are also handled by the CSS build system… it moves the files from whatever their location is in the repository into a different layout in the output directory (currently flat, but hierarchical structure is in the works).

> 
>> I suggest we go with a single-level directory structure. If people want to keep metadata about which tests have been reviewed that should not be encoded in the filesystem heirachy. I doubt we will do any better than assuming that tests are good and that implementors will file bugs when they find a bad test.
> 
> The test framework supports flags. We could certainly ask people to add a "reviewed" flag when they actually review a test (alternatively we can detect this with the <link rel=reviewer> convention). That should provide us with enough information to do whatever we need to do, e.g. generate a suite run containing only reviewed tests if someone needs that for some reason.

Please take a look at Shepherd[1], it already uses the <link rel=reviewer> metadata to mark tests as approved. Note that it knows the difference between reviewers who have the right to officially mark test as 'approved' versus simply 'accepted'. It also tracks review links per revision in the repository (and in different branches), if the test gets modified after the review, the approved status can get revoked (depending on who modified it).

Shepherd was purpose built for the job of tracking and managing metadata of tests and associated files. It's also a test bug tracker and repository browser/search tool.

> 
> Overall I agree that the approval step is a bureaucratic speed bump that is not being helpful. I think that we should move to a commit-then-review model in which people who have an interest in passing a test suite can file bugs against broken tests. Ideally, we would make flagging broken tests easy — I'm thinking about ways of doing that in the framework (suggestions welcome — I wonder if I could just add a flag to "reported as broken" test cases).

The framework already has the support for that, though no UI (yet). A possible result is 'invalid', which marks the test as bad until it is modified. Currently invalid results can only be entered by importing implementation reports. I'm in the process of folding Shepherd's user login system into the framework (they use the same core library) and will add a UI for logged in users to mark tests as broken. I'm also going to add links between the framework and Shepherd so users can go right to the page in Shepherd to file a bug against the test.

Peter

[1] Currently running at http://test.csswg.org/shepherd/

Received on Thursday, 31 May 2012 15:31:44 UTC