Fix and clean up load-error handling, remove Orchestra #233
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
People can bring their own Orchestra if they want it
Include a reduced version of the full event, instead of only the type.
(Toolkit/getDefaultToolkit) returns a SunToolkit, which is not publicly accessible. Instead the `getImage` method should be called on the abstract base class `java.awt.Toolkit`. We need a type hint on the string as well, to disambiguate the overloaded method call. Fixes this: WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by clojure.lang.InjectedInvoker/0x000000084007b840 (file:/home/arne/.m2/repository/org/clojure/clojure/1.10.3/clojure-1.10.3.jar) to method sun.a wt.SunToolkit.getImage(java.lang.String) WARNING: Please consider reporting this to the maintainers of clojure.lang.InjectedInvoker/0x000000084007b840 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release
It gets in the way too easily, at this point it's causing us a lot of work without adding much value.
This makes kaocha.watch use the mechanisms already available in `kaocha.testable` to deal with and report load errors. It also does a general pass of cleaning things up, because we had created a bit of a mess. `handle-load-error` is gone, the testable implementations should no longer need this, just let kaocha.testable handle this stuff for you. It also seems at some point we went from `:kaocha.test-plan/load-error` to `:kaocha.testable/load-error`, and ended up with a bit of a hodge-podge. It's surprising things still worked as well as they did. I changed the remaining test-plan/load-error references to testable/load-error, since that now dominates, and is IMO also the more intuitive, since we apply these on individual testables (typically the suite).
|
Thanks for the quick fix! It works great. Testing methodology: Gave this a try using On master I see the behavior describe in #226 (comment) On this branch, I get a stacktrace, which is the expected behavior |
Codecov Report
@@ Coverage Diff @@
## main #233 +/- ##
==========================================
- Coverage 77.88% 77.72% -0.16%
==========================================
Files 49 50 +1
Lines 2550 2546 -4
Branches 246 243 -3
==========================================
- Hits 1986 1979 -7
- Misses 416 418 +2
- Partials 148 149 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes kaocha.watch use the mechanisms already available in
kaocha.testableto deal with and report load errors. It also does a generalpass of cleaning things up, because we had created a bit of a mess.
handle-load-erroris gone, the testable implementations should no longer needthis, just let kaocha.testable handle this stuff for you.
It also seems at some point we went from
:kaocha.test-plan/load-errorto:kaocha.testable/load-error, and ended up with a bit of a hodge-podge. It'ssurprising things still worked as well as they did. I changed the remaining
test-plan/load-error references to testable/load-error, since that now
dominates, and is IMO also the more intuitive, since we apply these on
individual testables (typically the suite).
I removed the Orchestra instrumentation, and disabled the fdef check plugin. These things get in the way too easily, at this point they are causing us a lot of work without adding much value. I also dropped the (outdated) Orchestra dependency. The orchestra plugin is still there, but going forward people should bring their own Orchestra if they still want to use it.
Other small bits
Closes #226