I've chatted with @jieyouxu and we've come to the conclusion that it's worthwhile to track compiletest issues even though we plan on fully migrating to ui_test eventually since that might still take a while.
When parsing the revisions header / directive, compiletest naively splits the payload by whitespace leading to the bizarre situation that //@ revisions: foo, bar gets understood as declaring the two revisions foo, (notice the trailing comma!) and bar.
We should either throw an error (my favored solution) or permit , to be valid separator next to whitespace (less desirable in my opinion).
If we go with the first approach we should probably go all-in and restrict revision names to the regex [[:alpha:]_\-][[:alnum:]_\-]* (*) (ASCII-only or Unicode, shouldn't matter) and we can probably also emit the hint “commas not permitted, use whitespace” if we stumble upon a comma.
(*) Restricting the grammar of revision names also helps with portability. Windows is way stricter about file paths than *nix OSes and since compiletest generates files of the form <STEM>.<REVISION>.<STDSTREAM> revisions containing special characters may lead to tests passing locally for a contributor working on a *nix machine but failing on a Windows machine. CI would catch that but still ^^'
Similarly, compiletest permits whatever garbage you put between [ and ] in //@[...] DIRECTIVE, even //@[] is allowed. We should probably restrict the content to be identifier-like, too.
I've chatted with @jieyouxu and we've come to the conclusion that it's worthwhile to track
compiletestissues even though we plan on fully migrating toui_testeventually since that might still take a while.When parsing the
revisionsheader / directive,compiletestnaively splits the payload by whitespace leading to the bizarre situation that//@ revisions: foo, bargets understood as declaring the two revisionsfoo,(notice the trailing comma!) andbar.We should either throw an error (my favored solution) or permit
,to be valid separator next to whitespace (less desirable in my opinion).If we go with the first approach we should probably go all-in and restrict revision names to the regex
[[:alpha:]_\-][[:alnum:]_\-]*(*) (ASCII-only or Unicode, shouldn't matter) and we can probably also emit the hint “commas not permitted, use whitespace” if we stumble upon a comma.(*) Restricting the grammar of revision names also helps with portability. Windows is way stricter about file paths than *nix OSes and since
compiletestgenerates files of the form<STEM>.<REVISION>.<STDSTREAM>revisions containing special characters may lead to tests passing locally for a contributor working on a *nix machine but failing on a Windows machine. CI would catch that but still ^^'Similarly,
compiletestpermits whatever garbage you put between[and]in//@[...] DIRECTIVE, even//@[]is allowed. We should probably restrict the content to be identifier-like, too.