Repository navigation
Ensure unskipped testable for reporting load error - #371
alysbrooks merged 22 commits into
Conversation
Codecov ReportBase: 75.32% // Head: 75.19% // Decreases project coverage by
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
Flags with carried forward coverage won't be shown. Click here to find out more.
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. |
|
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. |
|
@alysbrooks sure, I went for the minimal fix. I can implement your suggestion. |
|
@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 |
alysbrooks
left a comment
alysbrooks
left a comment
There was a problem hiding this comment.
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.
| ::testable/load-error-file (or file (util/ns-file error-ns)) | ||
| ::testable/load-error-line line | ||
| ::testable/load-error-message (str "Failed reloading " error-ns ":"))) | ||
| (assoc suite ::testable/skip true)))) |
There was a problem hiding this comment.
Just to make sure I understand, this line is required to retain the "skip the rest" behavior?
|
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 |
|
Yep sure I'll write some tests. |
|
@alysbrooks ok done. I made a little framework that should make these kinds of tests easier. |
|
Scratch that, the test also passes on the master branch. Need to revise. |
|
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
left a comment
alysbrooks
left a comment
There was a problem hiding this comment.
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!
|
@alysbrooks thanks for the review, I think I've addressed everything. |
|
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. |
|
As a workaround, you can apply |
|
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. |
|
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. |
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.