Allow --pattern|-p option to repeat

[rhendric]

The effect of multiple -p options is the same as &&-ing the expressions together, but allowing them to be specified in separate options makes scripting test commands simpler.

---

Motivation: I want to run tests with a small shell script such as the following:

#!/usr/bin/env bash
default_test_options=(...)
test-exe "${default_test_options[@]}" "$@"

If default_test_options contains a pattern filter like -p ! /very-slow-test/, then I'm currently unable to provide any options to the script to further focus on a set of interest. I would have to write some extra logic to the script to detect pattern filters and inject them into the default pattern filter, making the script much more complicated than I would like it to be.

Allowing multiple -p options to be provided to Tasty programs resolves the issue.

[Bodigrim]

I like the idea, but I don't think you need to change the definition of TestPattern, this is disruptive. Instead you should be able to use foldl' Test.Tasty.Patterns.Types.And immediately in optionCLParser, combining everything into a single Expr.

[rhendric]

Good idea, that's a much smaller change.

[andreasabel]

Mmh, somehow I liked the [Expr] better here (with a comment that it is to be understood conjunctively) because this better reflects what is going on here.
Of course, this would be an API change then and require a major version bump.

[Bodigrim]

Unfortunately, [Expr] triggers changes in many other functions throughout the package, which is way too much breakage IMO.

[Bodigrim]

Just to be on the safe side and avoid a partial function, how about foldl' testPatternAnd noProgress instead of foldl1' testPatternAnd? I understand that some returns a non-empty list, but this unfortunately not reflected in types.

[andreasabel]

I think that some (as opposed to many) returns a non-empty list is an essential invariant one should rely on and reflect this in its use. So foldl1' is the right choice imho.
Not exploiting the invariants blurs the picture imo and leads to less readable code.

[Bodigrim]

Ideally it should be Data.Foldable1.foldl1' and Options.Applicative.NonEmpty.some1. We cannot use the former, because it's too new (just added in base-4.18), but some1 is probably a good choice to return a proper type.

@rhendric sorry for bikeshedding of this line :)

[rhendric]

Not having Foldable1.foldl1', which of the following should I use?

• Data.List.foldl1': takes a list, not a NonEmpty; so either I can't use some1, or I have to convert it to a list in the middle
• Data.Foldable.foldl1: not strict, probably a terrible idea, I'm only mentioning it out of completeness
• Data.Foldable.foldr1: I'm inclined to use this if you want me to use some1, but since you suggested foldl' first I want to be sure we're on the same page

[Bodigrim]

CC @martijnbastiaan for review

[martijnbastiaan]

Cool, I've wanted this myself before! Two things:

• I do think this deserves an entry in core/CHANGELOG.md
• If it's not too much trouble, I think one or two simple tests should be added to core-tests

[andreasabel]

My intuitive understanding of passing -p several times would be that any test matching any of the patterns is run. This would be disjunction. If I am not mistaken, this PR implements the conjunction, which I find unintuitive.
Since there are two monoids (conjunction and disjunction), and people have different ideas which one should be the default, I think we should not arbitrarily decide on either.

Making an option monoidal also violates the invariant that each option can equivalently be given by an environment variable. But you cannot have monoidal env vars.

tasty/README.md

Lines 245 to 247 in 959fe91

	Every option can be passed via environment. To obtain the environment variable
	name from the option name, replace hyphens `-` with underscores `_`, capitalize
	all letters, and prepend `TASTY_`. For example, the environment equivalent of

So, unfortunately, my judgement is "reject".

[rhendric]

Since there are two monoids (conjunction and disjunction), and people have different ideas which one should be the default, I think we should not arbitrarily decide on either.

But it isn't arbitrary. Of the two monoids, only conjunction has a mempty consistent with what Tasty does when no -p options are provided: run all the tests.

But you cannot have monoidal env vars.

I don't understand this objection. Sure, the interface of environment variables doesn't let you specify a variable multiple times. But it's still the case that anything you can specify with command line options can also be specified with env vars. It's also still the case that environment variables are overridden if any matching options are provided on the command line. What problems would this cause?

[andreasabel]

Of the two monoids, only conjunction has a mempty consistent with what Tasty does when no -p options are provided: run all the tests.

Ok, this is an argument for the bias you are implementing here. I am convinced.

[andreasabel]

I am fine with this feature in general, but it needs to be accompanied by tests and documentation updates.

[andreasabel]

It feels that the Nothing cases should be impossible.
However, there is the weird behavior parseTestPattern "" = Just noPattern.

tasty/core/Test/Tasty/Patterns.hs

Lines 57 to 60 in c2cefb1

	parseTestPattern :: String -> Maybe TestPattern
	parseTestPattern s
	| null s = Just noPattern
	| otherwise = TestPattern . Just <$> parseExpr s

I would have expected parseTestPattern "" = Nothing (or even parseTestPattern "" = undefined), because

The option -p expects an argument.

I would argue that the impossible cases should be marked in the source code as such,

testPatternAnd (TestPattern (Just x)) (TestPattern (Just y)) = TestPattern (Just (And x y))
testPatternAnd _ _ = undefined

and covered by tests.

[Bodigrim]

mkOptionCLParser is too restrictive, but it should be possible to avoid concatenation of Maybe Expr entirely with something like

optionCLParser = optional $ fmap (foldl1' testPatternAnd) $ some $ bla-bla-bla (short 'p' <> metavar "PATTERN")

for a certain bla-bla-bla ;)

[rhendric]

I don't think that's possible, if you intend to preserve the parseTestPattern "" = Just noPattern behavior. The parse action can return a value or raise an error; if the string it receives is empty, it can't return a Nothing (or we're stuck catenating Maybes), it can't return a TestPattern (because that wraps Maybes), and it can't return an Expr (unless you want me to stuff the empty string into an ERE and then test it outside before Anding it to another Expr, which is just like catenating Maybes but worse). And it can't raise an error because that would be inconsistent with what parseTestPattern does. What's left for it to do in that case?

[rhendric]

How and where should tests be written for this? core-tests looks a little idiosyncratic; am I supposed to test this by creating a new executable in core-tests.cabal with accompanying .hs and .sh files? And then how are those things invoked?

[Bodigrim]

am I supposed to test this by creating a new executable in core-tests.cabal with accompanying .hs and .sh files?

That would be fine from my perspective.

And then how are those things invoked?

Please check .github/workflows/ci.yml.

[rhendric]

I've written tests, added a changelog entry, and taken another approach with optionCLParser that will probably make everyone angry.

[Bodigrim]

...and taken another approach with optionCLParser that will probably make everyone angry.

I'm actually quite happy with it :) @andreasabel @martijnbastiaan opinions?

[Bodigrim]

@andreasabel just a gentle reminder to review, I'd like to wrap up things for a release soon.

[Bodigrim]

@rhendric could you please attach a comment to the instance IsOption TestPattern saying "Since tasty-1.5 this command-line option can be specified multiple times with accumulating effect bla-bla-bla" or similar?

[rhendric]

Done.

[andreasabel]

Thanks for the test!
I tried cabal run multiple-pattern-test locally.

[Bodigrim]

Thanks @rhendric!