Re: Towards a better testsuite: Review/Commit Policy

On 03/24/2016 01:00 PM, Geoffrey Sneddon wrote:
>
> The other notable difference is in tooling: Mercurial is used with a
> git mirror, and then reviews are done split across Shepherd and
> public-css-testsuite and some issues filed on GitHub, and with some
> people expecting some pre-landing review through GitHub PRs and with
> some people pushing directly to Mercurial… Really everything would be
> simpler if we had *one* single way to do things. I'd much rather have
> everything on GitHub, review happening on PR submission, and nits and
> such like be reported as GitHub issues. This keeps everything in one
> place with tools most people are used to.
>
> To outline what I'd like to see happen:
>
> - Get rid of the build system, replacing many of it's old errors with
> a lint tool that tests for them.
> - Policy changes to get rid of all metadata in the common case.
> - Change the commit policy. (Require review first, require no new lint errors.)

I agree with having a lint tool and changing the commit policy
to be consistent between repositories. I'm not sure "require
review first" is a good policy though; it's easier to review
tests that have landed, and it also means we can easily take
over fixing the test if the submitter is MIA. Also in many cases
(e.g. for web dev contributors who aren't planning to stick around,
but who want a particular issue tested) it's just more expedient
to just fix the test, and explain what was fixed, than cycling
through feedback.

So I'd propose just requiring no new lint errors, and allowing
individual contributors to land new tests without review,
possibly in a separate "unreviewed" directory. Contributors
without write access would need a quick security review before
landing. (Either no scripting, or the scripting is not malicious.)

Once a test has been reviewed it would be considered part of
the test suite; but unreviewed tests would be available for
running as well, and as part of that process are more likely
to get reviewed.

Changes to a test should get review. I agree though that having
the original test writer do that review is not workable. It's
nice to flag that person for the review if they are active (e.g.
gtalbot), but not if they're going to ignore the pull request
(e.g. contractors who have moved on to other things).


One thing the current system does track, which would be useful
not to lose, is which tests have been competently reviewed vs
those which have merely been rubber-stamped. We have a lot of
tests that were merely rubber-stamped during the CSS2.1 cycle,
and are therefore of questionable correctness / utility. It
might make sense to simply mark the rubber-stamped tests as
such using the <link> tag.

~fantasai

Received on Friday, 8 April 2016 17:01:10 UTC