Fail tests marked with .only if running in CI #598
Conversation
|
It seems the default branch changed to main after I did a fork so I just had to update the main branch on my fork to be main too. |
callado4
force-pushed
the
issue-590-fail-only-on-ci
branch
from
fba05e4 to
7b1c57d
Compare
| private[this] def onlyNotOnCiFailure(test: TestName): TestOutcome = { | ||
| val result = Result.Failure( | ||
| msg = "'Only' tag is not allowed when `isCI=true`", | ||
| source = None, | ||
| location = List(test.location) | ||
| ) | ||
| TestOutcome( | ||
| name = test.name, | ||
| duration = FiniteDuration(0, "ns"), | ||
| result = result, | ||
| log = Chain.empty | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think this makes sense for making a failing test outcome, but if there is a better way, please let me know.
A few things that I wanted to clarify
- msg in the result is the same message used in Munit
- source in result is None, this was't caused by an exception so no need to provide an exception (right?)
- location in result, using the location of the test definition itself as a single entry, is this what is expected here?
- duration is TestOutcome set to 0 since this test didn't actually run - is this fine?
- I provided an empty chain for TestOutcome.log, but do we want something here?
There was a problem hiding this comment.
Yup, this all looks good to me :)
modules/core/src/weaver/suites.scala
Outdated
| else if (testsTaggedOnly.nonEmpty && isCI) { | ||
| val failureOutcomes = testsTaggedOnly | ||
| .map(_._1) | ||
| .map(onlyNotOnCiFailure) | ||
| Stream.emits(failureOutcomes).lift[F](effectCompat.effect) | ||
| } |
There was a problem hiding this comment.
Do we want the tests to fail if there would end up being no tests to run, but there was something tagged .only? For example, a suite with just one test that has both .only and .ignore would be caught in the first if condition here and so we would return a Stream.empty, i.e., no tests would be run at all (and the .only is ignored).
On the other hand, if there are any tests to be run, and any of the other tests are marked as .only and .ignore we will fail (we will report all tests marked with .only)
There was a problem hiding this comment.
I think I'd swap both conditions so that the isCI flag would be checked first, and result in a failure if enabled and in the presence of any .only. The reason is that the behaviour would be surprising in a later PR when tests are added and there might be a cognitive disconnect between the suite failing and the addition of .only.
|
Thanks ! |
Implements #590