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]

Codecov Report

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

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

@@            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%)	⬇️
unit	69.37% <0.00%> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/kaocha/watch.clj	73.85% <0.00%> (-1.74%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 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]

Looks good. I don't see volatiles used much (actually, I can't think of a use I've seen in actual Clojure code), but they seem appropriate here. I just have one question I'd like to get an answer to before merging.

[alysbrooks]

Just to make sure I understand, this line is required to retain the "skip the rest" behavior?

[frenchy64]

Yes exactly.

[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.

[alysbrooks]

Thank you for all your work on the tests! I think we might be able to use your helpers to test some other behaviors of --watch. I think after these changes, this'll be ready to merge!

[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.