Fix NoSuchFileException due to spaces in file paths in JUnitResultArc… #663
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
|
Can you please re-import the PR template and use that to lay out your description, its hard to follow right now: https://github.com/jenkinsci/.github/blob/master/.github/pull_request_template.md |
|
Your PR title doesn't seem to match the PR description? |
timja
reviewed
Nov 25, 2024
| public JUnitParser( | ||
| StdioRetention stdioRetention, | ||
| boolean keepProperties, | ||
| boolean allowEmptyResults, | ||
| boolean skipOldReports, | ||
| boolean keepTestNames) { | ||
| this(stdioRetention, keepProperties, allowEmptyResults, skipOldReports, keepTestNames, false); | ||
| } | ||
| // New Constructor with the additional parameter |
There was a problem hiding this comment.
please don't add comments like this
Suggested change
| // New Constructor with the additional parameter |
| @@ -226,7 +254,35 @@ public T invoke(File ws, VirtualChannel channel) throws IOException { | |||
| TestResult result; | |||
| String[] files = ds.getIncludedFiles(); | |||
| if (files.length > 0) { | |||
| // not sure we can rely seriously on those timestamp so let's take the smaller one... | |||
| // New sorting logic starts here | |||
There was a problem hiding this comment.
don't use words like new that gets old first, comments should be describing the why not the what generally as well...
| @@ -226,7 +254,35 @@ public T invoke(File ws, VirtualChannel channel) throws IOException { | |||
| TestResult result; | |||
| String[] files = ds.getIncludedFiles(); | |||
| if (files.length > 0) { | |||
| // not sure we can rely seriously on those timestamp so let's take the smaller one... | |||
| // New sorting logic starts here | |||
| List<File> fileList = new ArrayList<>(); | |||
There was a problem hiding this comment.
from here to line 279 it could all be expressed more simplify using a stream.
roughly (pseudocode):
files.stream()
.map(file => new File...)
.sorted(sortFunction handling if variable set)
.map(file -> back to relative path)
.collect(toList() maybe)
.toArray() | return sortTestResultsByTimestamp; | ||
| } | ||
|
|
||
| // Add the field and setter if needed |
There was a problem hiding this comment.
Suggested change
| // Add the field and setter if needed |
| @@ -539,6 +541,20 @@ public void setSkipOldReports(boolean skipOldReports) { | |||
|
|
|||
| private static final long serialVersionUID = 1L; | |||
|
|
|||
| @Override | |||
| public boolean isSortTestResultsByTimestamp() { | |||
| // Return the appropriate value or a default (e.g., false) | |||
There was a problem hiding this comment.
Suggested change
| // Return the appropriate value or a default (e.g., false) |
| @@ -482,16 +484,24 @@ public void testXxe() throws Exception { | |||
| String oobInUserContentLink = j.getURL() + "userContent/oob.xml"; | |||
| String triggerLink = j.getURL() + "triggerMe"; | |||
|
|
|||
| String xxeFile = this.getClass().getResource("testXxe-xxe.xml").getFile(); | |||
| String xxeFileContent = FileUtils.readFileToString(new File(xxeFile), StandardCharsets.UTF_8); | |||
| URL xxeResourceUrl = this.getClass().getResource("testXxe-xxe.xml"); | |||
There was a problem hiding this comment.
Are these changes related to your change? You can submit them separately with reasoning if not
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.
Added an option to import multiple JUnit XML files sorted by last modification date
When collecting multiple JUnit XML files, the JUnit plugin currently imports test results based on file name order. This can lead to incorrect ordering if the file names do not reflect the actual sequence in which the tests were executed.
This pull request introduces a new option sortTestResultsByTimestamp to the JUnit plugin. When enabled, the plugin will sort the JUnit XML files by their last modification timestamp (from oldest to newest) before importing them. This ensures that test results are processed in the order they were generated, providing a more accurate representation of test execution order.
Key Changes
Added a new boolean parameter sortTestResultsByTimestamp to the JUnitParser and JUnitResultArchiver.
Implemented sorting logic in the JUnitParser to sort test result files by last modification date when the option is enabled.
Updated the plugin configuration UI to include the new option.
Maintained backward compatibility by keeping the default behavior (sorting by file name) when the option is not enabled.
Testing done
Unit Tests
Added unit tests in JUnitParserTest and JUnitResultArchiverTest to verify that test result files are correctly sorted by last modification date when sortTestResultsByTimestamp is enabled.
Tested scenarios with multiple files having different and identical modification times.
Manual Testing
Deployed the modified plugin in a local Jenkins instance.
Created a test job that generates multiple JUnit XML files with varying modification timestamps.
Verified that when sortTestResultsByTimestamp is enabled, the test results are imported in the correct order.
Confirmed that when the option is disabled, the files are processed in file name order.
Build Verification
Ran mvn clean install to build the plugin and ensure all tests pass.
Checked for any compiler warnings or errors.
Submitter checklist
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
Issue Link: JENKINS-65532
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue