Ensure unskipped testable for reporting load error

[frenchy64]

Fix #370

If the first testable is already skipped in watch mode, then the load error is never reported. Tested successfully in the reproduction mentioned in #370.

[codecov[bot]]

Codecov Report

Base: 75.32% // Head: 75.19% // Decreases project coverage by -0.13% :warning:

Coverage data is based on head (ab2d804) compared to base (b979e29). Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #371 +/- ## ========================================== - Coverage 75.32% 75.19% -0.14% ========================================== Files 51 51 Lines 2736 2741 +5 Branches 256 256 ========================================== Hits 2061 2061 - Misses 514 519 +5 Partials 161 161 ``` | Flag | Coverage Δ | | |---|---|---| | integration | `56.97% <0.00%> (-0.11%)` | :arrow_down: | | unit | `69.37% <0.00%> (-0.13%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/lambdaisland/kaocha/pull/371?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/kaocha/watch.clj](https://codecov.io/gh/lambdaisland/kaocha/pull/371?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2thb2NoYS93YXRjaC5jbGo=) | `73.85% <0.00%> (-1.74%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[alysbrooks]

I ran through your example with the fix applied, and it worked. Thanks for providing that—it helps a lot! I also verified that, once the issue preventing reloads is fixed, the correct tests are still skipped. (If you're interested in the changes I made to test that scenario, I can submit a PR.)

This fix makes sense, although I wonder if a better algorithm would be to arbitrarily pick the first unskipped testable and put the load error on that. I think doing it that way is technically more correct (since we aren't activating a previously skipped testable), but I'm not sure there would be any user-facing impact. In theory, some plugin or hook could depend on whether tests are skipped, even in the event of a load error, so it seems preferable.

If you want to try that, @frenchy64, go ahead. Otherwise, I think I'll merge this change for now since I think it's an improvement.

[frenchy64]

@alysbrooks sure, I went for the minimal fix. I can implement your suggestion.

[frenchy64]

@alysbrooks I implemented your suggestion and checked that it still works. Now the message is improved to include the suite I selected (before it was ERROR in unit, which was actually skipped):

ERROR in checker (loads_once.clj:3)
Failed reloading loads-once:

[alysbrooks]

Actually, another thing. There are no tests for this change. Would you be interesting in writing some? It's an awkward change to write tests for, so I'm okay merging this without them. If you are interested, you might be able to model them after kaocha.watch-test. Thanks!

[frenchy64]

Yep sure I'll write some tests.

[frenchy64]

@alysbrooks ok done. I made a little framework that should make these kinds of tests easier.

[frenchy64]

Scratch that, the test also passes on the master branch. Need to revise.

[frenchy64]

Fixed it. I forgot it the reproduction needs to first have a passing test run before introducing the compilation error. The test now fails on master but passes on this branch.

[frenchy64]

@alysbrooks thanks for the review, I think I've addressed everything.

[frenchy64]

Hmm looks like the test can exceed CircleCI's 10 minute build limit: https://app.circleci.com/pipelines/github/lambdaisland/kaocha/1030/workflows/75013baa-25a5-4f2a-9aa9-3d697c185c21/jobs/6878

Not sure what to do TBH.

[alysbrooks]

As a workaround, you can apply ^{:min-java-version "1.11"} to the failing test (or I can). But it does seem to work sometimes.

[plexus]

Thanks a lot @frenchy64 for this PR and @alysbrooks for shepherding it to completion, I really appreciate the back and forth here to make sure it's good. Hiding load errors is extremely frustrating for users, so I'm glad we get to weed out these issues.

I see this introduces a new way of doing kaocha black box testing, this is normally what we have Cucumber for, which has the benefit of doubling as documentation. But Ambrose I appreciate you already put a lot of time into this so I'm fine with having this go in as-is. Maybe put a comment on the test saying that we'd like to move this to cucumber at some point?

A CHANGELOG entry would also be good. Apart from that all looks good.

[alysbrooks]

Thank you, @frenchy64!

I should have documented my reasoning for suggesting black box testing be done with Cucumber. If I remember correctly, my reasoning is that we already had integration tests for watch not part of Cucumber tests.