`skip` option accepts an empty string and skips all tests

HSPEC_SKIP="" appears to skip all tests and succeed. This caught me by surprise.

I was using the environment variable HSPEC_SKIP to conditionally skip particular tests during CI. Sometimes I'd pass in a value such as "name of my test to skip" and other times I'd pass in "" thinking this would run the full test suite. I was surprised that HSPEC_SKIP="" skips the entire test suite, as I expected that to behave as if the environment variable was unset.

I can see a few options:

1. Disallow "" as a value for skip during config argument parsing. Maybe using the existing "invalid argument" reporting functionality?
2. Silently coerce "" to behave as if the option wasn't provided. This would behave as if nothing was skipped (matching my expectation) and run the full test suite.
3. Keep the current functionality. Maybe this is intended? Maybe documenting this behavior could be helpful?
4. ???

I haven't tested myself, but I imagine using command line arguments --skip "" would behave similarly.

I think that is what a naive implementation (substring matching) gives you: The empty string is a substring of any other string. So while possibly surprising, it's somehow consistent. Note that this would also be true for regexp matching, say the empty expression // matches everything.

Looking at what other software does (non-comprehensive of course):

• In Vim %s///g will replace every line in the current buffer with an empty line.
• In bash:

  $ echo foo | grep ''
  foo
  

My thoughts for now: Not eager to do (2), as it is a breaking change and I can't even argue that it is more correct. So either (1) or (3), not sure.

I also think that it is a nice to have if --skip is inverse to --match. Say if --skip "" ignoring everything is consistent with --match "" matching everything.

@SamProtas ignoring --skip for a moment, does --match "" behave in a way you would intuitively expect?

Your explanation of why "" behaves this way makes sense to me. I think --match "" matching everything feels the same as the --skip "" situation in that it wasn't intuitive but makes sense once it's understood that matching is substring matching. I agree having consistent behavior is important.

It took reading the source code to learn that the word "match" in the docs (skip examples that match given PATTERN) meant isInfixOf. Testing that in ghci is how I finally resolved the issue I was having.

What is the utility of supporting "" for these arguments vs rejecting it (option 1 from above)?

1. This is the current behavior. Changing this might affect current users.
2. If you know what behavior it leads to and want that behavior, it might lead to cleaners scripts. Someone might have a script that calls something like stack test --test-arguments="--match '${OPTIONAL_MATCH_ARG}'" vs having to exclude --match if unused.
3. ???

What is the downside of supporting "" for these arguments?

1. Some people find this unintuitive. For those that misunderstand the meaning, specifically the meaning of HSPEC_SKIP=, the outcome is somewhat dangerous (thinking all of your tests ran successfully, when in reality none of them ran).
2. It's not necessary functionality. You could skip all tests by not running the test suite. You could match all tests by not using the --match flag.
3. ???

I'm somewhat struggling to articulate why my intuition was what it was. Anecdotally, several of my coworkers shared this intuition. I'll poll them for input here.

The semantics of skip and match having inverse behavior make sense to me, but this wasn't something I had thought about previously.

Even though grep and and vim/regex have similar semantics, this behavior is still unintuitive in the context of a test suite and caused us to skip CI in a production pipeline. No errors occurred. I think that generally, any time that zero tests are run in an automated testing environment, that's unexpected and should be considered an error. As Sam said, one can skip all tests by not running hspec at all.

One solution/workaround that would satisfy my worries of accidentally disabling CI is to add a new flag to hspec that instructs it to fail or output an error code if zero tests are run. I would even recommend that this become the default behavior, but understand that it would be a breaking change.

I agree with @ldub and @SamProtas that

It's not necessary functionality. You could skip all tests by not running the test suite.

One solution/workaround that would satisfy my worries of accidentally disabling CI is to add a new flag to hspec that instructs it to fail or output an error code if zero tests are run

This is a nice idea. I think it should be a default. If there's a non-empty test suite, but no tests are run, the program should not exit normally/silently.

I would also be in favor of changing the current behavior (as another user of hspec). The fact that hspec will silently ignore all the tests when HSPEC_SKIP is being (mis-)used in this way is surprising and not good.

My thoughts for now: Not eager to do (2), as it is a breaking change and I can't even argue that it is more correct. So either (1) or (3), not sure.

I think (1) is also a breaking change, since there could be people that have e.g. CI scripts that will break if hspec starts rejecting "" for HSPEC_SKIP. So I would argue that option (2) should be on the table. It's certainly the behavior that I would've expected before seeing this issue. (Maybe it's slightly better to start rejecting an argument value (option (1)) instead of just starting to behave differently (option (2)), but personally I think that's not a strong argument.)

In any case I'm not that opinionated on how this issue gets fixed, but I think it should be fixed. Either through option (1) or (2) or through @ldub's idea of failing whenever either HSPEC_SKIP or HSPEC_MATCH is used, but the set of selected tests turns out to be empty.

Btw, I just confirmed that the command line flags --skip and --match behave the same as the env vars.

Right now, I'm focusing on getting 2.10.0 out, but we will address this in some way post-2.10.0.

So far my best bet is to add a --strict flag (fail on empty test suite, but also fail on focused test, possibly pending test and empty descriptions) + possibly enabling it by default if CI=true.

(basically, a continuation of https://github.com/hspec/hspec/issues/389)

Along the lines of what @ldub suggested, we now have --fail-on=empty. It is, among other things, implied by --strict.

My recommendation is to use --strict / HSPEC_STRICT=yes on CI. Note however, that --strict is not a stable set of options; we will add to it as needed (e.g. #700).

As of now CI=true does not imply --strict. Personally, I always want it. I still think it would be too intrusive. My rational here is that if you are messing around with things like HSPEC_SKIP then you probably had a look at the docs and you had a reasonable chance to discover by yourself that you want --strict.

I would probably be ok with enabling --strict for sol/run-haskell-tests.

1. We need better output for --fail-on=empty (https://github.com/hspec/hspec/issues/736)
2. Hspec’s own test suite fails with --fail-on=empty, and by extension --strict

I’m going to remove empty from --strict until at least (1) is resolved.