Added a ToilCompatibleTest class to CLI #215
Conversation
This is necessary because lines like this https://github.com/dockstore/dockstore/blob/00cccd2a1afc47d51c06a877df003577305afa62/dockstore-common/src/main/java/io/dockstore/common/yaml/DockstoreYamlHelper.java#L184 will generate error messages that will confuse the end user. More user friendly messages are generated.
|
I do not have any reason to believe that this PR would cause bitbucket tests to fail. |
| if (!DEBUG.get() && !INFO.get()) { | ||
| // Switching log off if DEBUG or INFO is not set | ||
| // yamlClient will generate unnecessary error logs | ||
| root.setLevel(Level.OFF); |
There was a problem hiding this comment.
Kinda wonder if you can set the log level for a particular package or lower in the tree. Probably not too important since this only affects this specific command
There was a problem hiding this comment.
I didn't know this until looking at this PR, but there's already code above starting around line 677 that invokes root.setLevel() based on the command line switches, and it feels weird to me to set it to Level.OFF only for this particular path.
I'd suggest one of these:
- Doing
root.setLevel(Level.OFF)up above instead. - Don't use LOG.error for errors, use LOG.info
- Do a pattern something like this:
if (LOG.isDebugEnabled()) {
LOG.error("the error message", theException);
}
I dislike 2.
I lean towards 3 -- it seems a little counter-intuitive, but the nice thing about it is that it won't log by default, but if it does log, it's logging at the ERROR level, so it will stand out for developers. 1 seems a little severe, it's also possible.
Also, see dockstore/dockstore#5223, which relates to this one -- ideally the solution would work for both.
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov ReportBase: 69.27% // Head: 69.04% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #215 +/- ##
=============================================
- Coverage 69.27% 69.04% -0.24%
+ Complexity 1041 1026 -15
=============================================
Files 46 46
Lines 5940 5940
Branches 776 776
=============================================
- Hits 4115 4101 -14
Misses 1503 1503
- Partials 322 336 +14
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. |
coverbeck
left a comment
There was a problem hiding this comment.
While this change is good, the PR title nor linked issue seem applicable. Could you please either fold into your other PR, or update the title/description of this PR?
I have updated the title and description. |
Description
The CLI requires the use of the
ToilCompatibleTestclass. However,ToilCompatibleTestclass was recently removed fromdockstore/dockstorein this PR. Meaning that the CLI cannot run off the currentdockstore/dockstorerepo. This PR adds theToilCompatibleTestto the CLI.Review Instructions
To review this ticket, you should
dockstore/dockstorerepoSecurity
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
./mvnw clean install\