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-69036] Do not always log errors if input is empty #1419

Merged
merged 12 commits into from
Feb 1, 2023

Conversation

shamlymhd
Copy link
Contributor

@shamlymhd shamlymhd commented Dec 2, 2022

Do not log an error if input is empty for those parsers that can read from the console log: in those cases an empty input just means that there are no warnings, see JENKINS-69036 for details.

@uhafner uhafner changed the title Fix for JENKINS-69036 [JENKINS-69036] Do not always log errors if input is empty Dec 2, 2022
@uhafner uhafner added the enhancement Enhancement of existing functionality label Dec 2, 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.

It would be also have two tests, one for a parser that scans the console log and one for an XML parser. See bug report for the former one.

@shamlymhd
Copy link
Contributor Author

See bug report for the former one. -> All SpotBugs are empty. so where should I look for?

@uhafner
Copy link
Member

uhafner commented Dec 6, 2022

See bug report for the former one. -> All SpotBugs are empty. so where should I look for?

What I meant: we need a test case. One with the Doxgen Parser (see affected parser in the bug report https://issues.jenkins.io/browse/JENKINS-69036), and another one e.g. with CheckStyle. When you parse an empty log, then Doxgen will not report an error while CheckStyle does.

@uhafner uhafner mentioned this pull request Dec 7, 2022
6 tasks
@shamlymhd
Copy link
Contributor Author

This issue is closed. right?

@uhafner
Copy link
Member

uhafner commented Dec 12, 2022

This issue is closed. right?

Not yet. Each PR requires a test case, see my comments above.

@uhafner
Copy link
Member

uhafner commented Dec 12, 2022

Here is an example test case:

https://github.com/jenkinsci/warnings-ng-plugin/blob/master/plugin/src/test/java/io/jenkins/plugins/analysis/warnings/steps/StepsITest.java#L489

This one fails, when there are no files found. You need to create a similar one that reads an empty file.

When you enable your new option, the new test should not fail for Oxygen or JavaDoc. For a tool like PMD, CheckStyle, that read XML, there still should be an error when the input is empty.

@uhafner
Copy link
Member

uhafner commented Jan 23, 2023

@shamlymhd ping! Are you planning to finish this PR?

@shamlymhd
Copy link
Contributor Author

@uhafner yeah I had exams in the last two weeks and am back to this today.

@uhafner
Copy link
Member

uhafner commented Jan 23, 2023

Ok, no worries! I just wasn't sure if you are waiting for some input. So take all the time you need!

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #1419 (e59d206) into master (b8e85d1) will increase coverage by 0.72%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1419      +/-   ##
============================================
+ Coverage     79.83%   80.56%   +0.72%     
- Complexity     1473     1475       +2     
============================================
  Files           254      254              
  Lines          5644     5645       +1     
  Branches        425      426       +1     
============================================
+ Hits           4506     4548      +42     
+ Misses          986      945      -41     
  Partials        152      152              
Impacted Files Coverage Δ
...lugins/analysis/core/model/IssueReportScanner.java 86.66% <100.00%> (ø)
...lugins/analysis/core/model/ReportScanningTool.java 82.02% <100.00%> (+0.20%) ⬆️
...a/io/jenkins/plugins/analysis/warnings/RevApi.java 88.88% <0.00%> (+1.38%) ⬆️
...io/jenkins/plugins/analysis/warnings/FindBugs.java 100.00% <0.00%> (+7.69%) ⬆️
...a/io/jenkins/plugins/analysis/warnings/Simian.java 100.00% <0.00%> (+12.50%) ⬆️
...o/jenkins/plugins/analysis/warnings/DupFinder.java 100.00% <0.00%> (+12.50%) ⬆️
...kins/plugins/analysis/warnings/WarningsPlugin.java 71.42% <0.00%> (+14.28%) ⬆️
...ins/analysis/warnings/VeraCodePipelineScanner.java 100.00% <0.00%> (+14.28%) ⬆️
...kins/plugins/analysis/warnings/IdeaInspection.java 85.71% <0.00%> (+14.28%) ⬆️
...java/io/jenkins/plugins/analysis/warnings/Pit.java 100.00% <0.00%> (+16.66%) ⬆️
... and 33 more

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

@shamlymhd
Copy link
Contributor Author

how can I solve this issue?

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.

Just some small documentation improvements.

shamlymhd and others added 4 commits February 1, 2023 20:54
…s/StepsITest.java

Co-authored-by: Ullrich Hafner <ullrich.hafner@gmail.com>
…s/StepsITest.java

Co-authored-by: Ullrich Hafner <ullrich.hafner@gmail.com>
…s/StepsITest.java

Co-authored-by: Ullrich Hafner <ullrich.hafner@gmail.com>
…s/StepsITest.java

Co-authored-by: Ullrich Hafner <ullrich.hafner@gmail.com>
@uhafner uhafner merged commit 409e850 into jenkinsci:master Feb 1, 2023
@shamlymhd shamlymhd deleted the shamly-fix-ii branch February 2, 2023 02:01
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
Development

Successfully merging this pull request may close these issues.

2 participants