Log the time offset warning at INFO if time not exceeded #40
Conversation
This file contains 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
Currently, at the end of every suite run, the node's time offset warning is logged at WARN level. However, in the event that the allowed time offset is not exceeded, this does not require user action. This change modifies the reporting calls to log the message at INFO level if the time offset is not exceeded, and WARN if it is exceeded. This allows a user to set their log level to WARN, and receive the message only when they need to rebalance their knapsack timing report.
|
Nice work. Thanks! I've released knapsack 1.10.0. BTW If you would find some time to try http://knapsackpro.com then I would be grateful to get some feedback from you about knapsack_pro gem. I added recently dashboard on the page so you can generate multiple API tokens for each of your test suites. |
ArturT
added a commit
that referenced
this pull request
Jun 3, 2016
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.
Hi @ArturT - thanks for releasing the last logging PR, my colleagues are really happy with the cleaner test output. 👍
There's one more source of regular output that we'd like to suppress if possible, which is the time offset warning. Currently, at the end of every suite run, the node's time offset warning is logged at WARN level. However, if the allowed time offset is not exceeded, this doesn't require user action, so we'd quite like to suppress it.
This change modifies the reporting calls to log the message at INFO level if the time offset is not exceeded, and WARN if it is exceeded. This allows a user to set their log level to WARN, and receive the message only when they need to rebalance their knapsack timing report.
I hope the approach is okay - I considered pushing the logger calls into the
Presenterclass itself (because there's a degree of duplication between the different adapters), but that didn't seem like a presentational responsibility, so I left it as-is. If you'd prefer another solution, though, just let me know.