Mechanism for reftests filtering

[Keryan-dev]

This is an attempt to have a generalization to filter arbitrarily reftests, with the main objective being having a command that run quickly locally.

The approach is to use custom-defined tags in the testfiles and injecting corresponding variable in the dune command environment (in a way that doesn't affect exisiting behavior). The lack of expressivity in dune files limits filter expressivity but I think this is good enough as a first step toward something we'd want to use more systematically.

Also, this makes tests summary more broadly available to other rules

[kit-ty-kate]

otherwise lgtm

[kit-ty-kate]

String.split_on_char never returns an empty list. Instead it is [""] (see https://ocaml.org/manual/5.2/api/String.html#extract)

[Keryan-dev]

I can add a comment for this, or fix the function, but [] and [""] end up with the exact same result anyways.

[kit-ty-kate]

i think replacing it with assert false would be a bit nicer

[kit-ty-kate]

I think it's best to make the set of tags limited to make it easier to maintain, agree on what tags we allow and for the free typo detection

Suggested change

	| ar::_ as tags -> ar, tags
	in
	let os_condition = match Filename.extension base_name with
	| "" -> ""
	| os ->
	Printf.sprintf "(= %%{os_type} %S)"
	String.(capitalize_ascii (sub os 1 (length os - 1)))
	in
	let tags_conditions =
	List.map (fun tag ->
	Printf.sprintf
	"(= %%{env:TEST%s=0} 1)"
	tag)
	tags
	in
	| ar::tags -> ar, tags
	in
	let os_condition = match Filename.extension base_name with
	| "" -> ""
	| os ->
	Printf.sprintf "(= %%{os_type} %S)"
	String.(capitalize_ascii (sub os 1 (length os - 1)))
	in
	let tags_conditions =
	List.map (function
	| ("N0REP0") as tag ->
	Printf.sprintf
	"(= %%{env:TEST%s=0} 1)"
	tag
	| tag -> raise (Failure (Printf.sprintf "Tag '%s' unrecognized" tag)))
	(if String.equal ar null_hash then ar :: tags else tags)
	in

[Keryan-dev]

I'm not opposed to this, however right now we can only choose specific tests with a present tag. So we have to be wary of that if we want to maintain a proper tag list.

Also, repo hashes are also considered for filtering and those are kind of arbitrary in any case.

[kit-ty-kate]

Also, repo hashes are also considered for filtering and those are kind of arbitrary in any case.

yeah, i removed that handling in my suggestion because i thought it wasn't useful and complicates the list of environment variable that affects opam

So we have to be wary of that if we want to maintain a proper tag list.

i think this PR is good way to get the discussion started, but you're right, I've updated the suggestion to only handle N0REP0

[Keryan-dev]

I feel we should not include repo hash to the tags in that case, as we won't have a reliable way to concile restrictions on both. Adding a first tag for the proposed use-case (that coincidentally appears along N0REP0) would be more orderly.

[kit-ty-kate]

As mentioned on slack i don't think we should have a "quick" tag, as maintaining it is more work than it's worth and its meaning can't really be defined either. What was bad about using N0REP0 like my code suggestion above?

[Keryan-dev]

I'm not against using repo hashes as filters but I think it doesn't fit well with tag restrictions, because we won't apply those to repo hashes (at least not in a pretty way). And making N0REP0 the unique exception feels unsatisfactory.

We could constraint ourselves to using only well-defined tags, but in that case hashes don't really fit either, neither my initial goal of having an informal selection of tests for fast development feedback.

[Keryan-dev]

Incorporated Kate's suggestion.
We can keep it that way until more use-cases for tags appear. Fiddling with the constraints should be relatively painless in futur patches either way.

[rjbou]

Thanks!

[kit-ty-kate]

Thanks