Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-69907] Suppress logging with optional param #1416

Merged
merged 17 commits into from Dec 14, 2022

Conversation

viradan
Copy link
Contributor

@viradan viradan commented Nov 30, 2022

Goal: reduce console output in jenkins job when printing file name and num of issues found.
Change: introduced new param quiet to mute logging.

See: JENKINS-69907

Comments for initial suggestion with sysproperty - #1403

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@uhafner uhafner changed the title suppress logging with optional param Suppress logging with optional param Nov 30, 2022
@uhafner uhafner added the enhancement Enhancement of existing functionality label Nov 30, 2022
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #1416 (dbe52bf) into master (6c180a8) will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #1416      +/-   ##
============================================
+ Coverage     79.87%   79.88%   +0.01%     
- Complexity     1452     1461       +9     
============================================
  Files           252      252              
  Lines          5594     5622      +28     
  Branches        424      425       +1     
============================================
+ Hits           4468     4491      +23     
- Misses          976      981       +5     
  Partials        150      150              
Impacted Files Coverage Δ
...plugins/analysis/core/steps/ScanForIssuesStep.java 85.24% <40.00%> (-2.48%) ⬇️
...plugins/analysis/core/steps/PublishIssuesStep.java 57.14% <71.42%> (+0.35%) ⬆️
...ns/plugins/analysis/core/steps/IssuesRecorder.java 68.01% <90.90%> (+0.59%) ⬆️
...ins/plugins/analysis/core/steps/IssuesScanner.java 93.54% <100.00%> (+0.08%) ⬆️
.../plugins/analysis/core/steps/RecordIssuesStep.java 60.66% <100.00%> (+0.95%) ⬆️
...jenkins/plugins/analysis/core/util/LogHandler.java 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@viradan viradan requested a review from uhafner December 6, 2022 13:52
@uhafner uhafner changed the title Suppress logging with optional param [JENKINS-69907] - Suppress logging with optional param Dec 7, 2022
@uhafner uhafner changed the title [JENKINS-69907] - Suppress logging with optional param [JENKINS-69907] Suppress logging with optional param Dec 7, 2022
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks almost good now! Thanks for improving.

One little thing would be helpful: Change one of the tests in StepsITest so that it runs one time with quite and one time without. And then check the console log accordingly. (Example in test shouldSkipBlaming)

@viradan
Copy link
Contributor Author

viradan commented Dec 8, 2022

@uhafner many thx for support! I'd possibly need one more hint, here is wrong assumption. Not sure what would be best...

  • what part of getConsoleLog(baseline) is output log only? Looks like even pipeline snippet is included
  • or add & check new log line for quiet option (eg smthg like "Logging is suppressed as requested")?

+ " }\n"
+ "}", true));
Run<?, ?> baseline = buildSuccessfully(job);
assertThat(getConsoleLog(baseline)).hasSize(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no way to suppress or work on the whole log. I think you simply should

assertThat(getConsoleLog(baseline)).doesNotContain("Searching for all files in");

You can also use a ParameterizedTest with a ValueSource(booleans = {true, false} and then use the same test:

if (isQuite) {
   assertThat(getConsoleLog(baseline)).doesNotContain("Searching for all files in");
} 
else {
   assertThat(getConsoleLog(baseline)).contains("Searching for all files in");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I get it right, does console log really contains LogHandler output or its feeded separately without LogHandler? Whatever I've tried console contained all content.
Also, the other option add & check new log line would make it not completely muted (not a big issue, but...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed now, there was still one method left that had no guard. Now the guard is at the lowest method in the call stack.

@viradan
Copy link
Contributor Author

viradan commented Dec 14, 2022

Is there direct correlation between description here and suppressing here - like error logger should not be suppressed?

@uhafner
Copy link
Member

uhafner commented Dec 14, 2022

Is there direct correlation between description here and suppressing here - like error logger should not be suppressed?

You are right, I copied the text from another plugin. I fixed the text now.

@uhafner uhafner self-requested a review December 14, 2022 10:35
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@viradan
Copy link
Contributor Author

viradan commented Dec 14, 2022

thank you! 👍

@uhafner uhafner merged commit b4dc08e into jenkinsci:master Dec 14, 2022
@uhafner
Copy link
Member

uhafner commented Dec 14, 2022

Thanks for your pull request! This is a much more versatile approach as the first one.

I will extract the main parts of this PR to apply the same pattern for my other plugins (code coverage).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
2 participants