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.

Sign up for GitHub

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

Fix NoSuchFileException due to spaces in file paths in JUnitResultArc… #663

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix NoSuchFileException due to spaces in file paths in JUnitResultArc… #663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 74 additions & 14 deletions src/main/java/hudson/tasks/junit/JUnitParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
import io.jenkins.plugins.junit.storage.JunitTestResultStorage;
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.MasterToSlaveFileCallable;
Expand All @@ -58,11 +62,12 @@
private final boolean keepTestNames;

private final boolean skipOldReports;
private final boolean sortTestResultsByTimestamp;

/** Generally unused, but present for extension compatibility. */
@Deprecated
public JUnitParser() {
this(false, false, false, false);
this(StdioRetention.DEFAULT, false, false, false, false, false);
}

/**
Expand All @@ -71,42 +76,60 @@
*/
@Deprecated
public JUnitParser(boolean keepLongStdio) {
this(keepLongStdio, false, false, false);
this(StdioRetention.fromKeepLongStdio(keepLongStdio), false, false, false, false);
}

/**
* @param keepLongStdio if true, retain a suite's complete stdout/stderr even if this is huge and the suite passed
* @param allowEmptyResults if true, empty results are allowed
* @since 1.10
*/
@Deprecated
public JUnitParser(boolean keepLongStdio, boolean allowEmptyResults) {
this(StdioRetention.fromKeepLongStdio(keepLongStdio), false, allowEmptyResults, false, false);
this(StdioRetention.fromKeepLongStdio(keepLongStdio), false, allowEmptyResults, false, false, false);
}

@Deprecated
public JUnitParser(
boolean keepLongStdio, boolean keepProperties, boolean allowEmptyResults, boolean skipOldReports) {
this(StdioRetention.fromKeepLongStdio(keepLongStdio), keepProperties, allowEmptyResults, skipOldReports, false);
this(
StdioRetention.fromKeepLongStdio(keepLongStdio),
keepProperties,
allowEmptyResults,
skipOldReports,
false,
false);
}

@Deprecated
public JUnitParser(
StdioRetention stdioRetention, boolean keepProperties, boolean allowEmptyResults, boolean skipOldReports) {
this(stdioRetention, keepProperties, allowEmptyResults, skipOldReports, false);
this(stdioRetention, keepProperties, allowEmptyResults, skipOldReports, false, false);
}

@Deprecated
public JUnitParser(
StdioRetention stdioRetention,
boolean keepProperties,
boolean allowEmptyResults,
boolean skipOldReports,
boolean keepTestNames) {
this(stdioRetention, keepProperties, allowEmptyResults, skipOldReports, keepTestNames, false);
}

Check warning on line 118 in src/main/java/hudson/tasks/junit/JUnitParser.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 79-118 are not covered by tests
// New Constructor with the additional parameter
Copy link
Member

Choose a reason for hiding this comment

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

please don't add comments like this

Suggested change
// New Constructor with the additional parameter

public JUnitParser(
StdioRetention stdioRetention,
boolean keepProperties,
boolean allowEmptyResults,
boolean skipOldReports,
boolean keepTestNames,
boolean sortTestResultsByTimestamp) {
this.stdioRetention = stdioRetention;
this.keepProperties = keepProperties;
this.allowEmptyResults = allowEmptyResults;
this.keepTestNames = keepTestNames;
this.skipOldReports = skipOldReports;
this.keepTestNames = keepTestNames;
this.sortTestResultsByTimestamp = sortTestResultsByTimestamp;
}

@Override
Expand Down Expand Up @@ -152,7 +175,8 @@
keepTestNames,
pipelineTestDetails,
listener,
skipOldReports));
skipOldReports,
sortTestResultsByTimestamp));
}

public TestResultSummary summarizeResult(
Expand All @@ -174,7 +198,8 @@
pipelineTestDetails,
listener,
storage.createRemotePublisher(build),
skipOldReports));
skipOldReports,
sortTestResultsByTimestamp));
}

private abstract static class ParseResultCallable<T> extends MasterToSlaveFileCallable<T> {
Expand All @@ -194,6 +219,7 @@
private final TaskListener listener;

private boolean skipOldReports;
private final boolean sortTestResultsByTimestamp;

private ParseResultCallable(
String testResults,
Expand All @@ -204,7 +230,8 @@
boolean keepTestNames,
PipelineTestDetails pipelineTestDetails,
TaskListener listener,
boolean skipOldReports) {
boolean skipOldReports,
boolean sortTestResultsByTimestamp) {
this.buildStartTimeInMillis = build.getStartTimeInMillis();
this.buildTimeInMillis = build.getTimeInMillis();
this.testResults = testResults;
Expand All @@ -216,6 +243,7 @@
this.pipelineTestDetails = pipelineTestDetails;
this.listener = listener;
this.skipOldReports = skipOldReports;
this.sortTestResultsByTimestamp = sortTestResultsByTimestamp;
}

@Override
Expand All @@ -226,7 +254,35 @@
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
Copy link
Member

Choose a reason for hiding this comment

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

don't use words like new that gets old first, comments should be describing the why not the what generally as well...

List<File> fileList = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

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()

for (String fileName : files) {
fileList.add(new File(ds.getBasedir(), fileName));
}

if (sortTestResultsByTimestamp) {

Check warning on line 263 in src/main/java/hudson/tasks/junit/JUnitParser.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 263 is only partially covered, one branch is missing
Collections.sort(fileList, Comparator.comparingLong(File::lastModified));

Check warning on line 264 in src/main/java/hudson/tasks/junit/JUnitParser.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 264 is not covered by tests
} else {
Collections.sort(fileList, Comparator.comparing(File::getName));
}

// Convert back to String array with paths relative to the base directory
String[] sortedFiles = new String[fileList.size()];
String baseDirPath = ds.getBasedir().getAbsolutePath();
for (int i = 0; i < fileList.size(); i++) {
String absolutePath = fileList.get(i).getAbsolutePath();
if (absolutePath.startsWith(baseDirPath)) {

Check warning on line 274 in src/main/java/hudson/tasks/junit/JUnitParser.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 274 is only partially covered, one branch is missing
sortedFiles[i] = absolutePath.substring(baseDirPath.length() + 1);
} else {
sortedFiles[i] = absolutePath;

Check warning on line 277 in src/main/java/hudson/tasks/junit/JUnitParser.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 277 is not covered by tests
}
}

// Update the DirectoryScanner with the sorted files
ds.setIncludes(sortedFiles);

// Continue with existing processing logic
// Not sure we can rely seriously on those timestamps so let's take the smaller one...
long filesTimestamp = Math.min(buildStartTimeInMillis, buildTimeInMillis);
// previous mode buildStartTimeInMillis + (nowSlave - nowMaster);
if (LOGGER.isLoggable(Level.FINE)) {
Expand Down Expand Up @@ -270,7 +326,8 @@
boolean keepTestNames,
PipelineTestDetails pipelineTestDetails,
TaskListener listener,
boolean skipOldReports) {
boolean skipOldReports,
boolean sortTestResultsByTimestamp) {
super(
testResults,
build,
Expand All @@ -280,7 +337,8 @@
keepTestNames,
pipelineTestDetails,
listener,
skipOldReports);
skipOldReports,
sortTestResultsByTimestamp);
}

@Override
Expand All @@ -303,7 +361,8 @@
PipelineTestDetails pipelineTestDetails,
TaskListener listener,
JunitTestResultStorage.RemotePublisher publisher,
boolean skipOldReports) {
boolean skipOldReports,
boolean sortTestResultsByTimestamp) {
super(
testResults,
build,
Expand All @@ -313,7 +372,8 @@
keepTestNames,
pipelineTestDetails,
listener,
skipOldReports);
skipOldReports,
sortTestResultsByTimestamp);
this.publisher = publisher;
}

Expand Down
32 changes: 24 additions & 8 deletions src/main/java/hudson/tasks/junit/JUnitResultArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ private static TestResult parse(
task.isKeepProperties(),
task.isAllowEmptyResults(),
task.isSkipOldReports(),
task.isKeepTestNames())
task.isKeepTestNames(),
task.isSortTestResultsByTimestamp())
.parseResult(expandedTestResults, run, pipelineTestDetails, workspace, launcher, listener);
}

Expand All @@ -189,7 +190,7 @@ protected TestResult parse(
}

@Override
public void perform(Run build, FilePath workspace, Launcher launcher, TaskListener listener)
public void perform(Run<?, ?> build, FilePath workspace, Launcher launcher, TaskListener listener)
throws InterruptedException, IOException {
if (parseAndSummarize(this, null, build, workspace, launcher, listener).getFailCount() > 0
&& !skipMarkingBuildUnstable) {
Expand Down Expand Up @@ -288,7 +289,8 @@ public static TestResultSummary parseAndSummarize(
task.isKeepProperties(),
task.isAllowEmptyResults(),
task.isSkipOldReports(),
task.isKeepTestNames())
task.isKeepTestNames(),
task.isSortTestResultsByTimestamp())
.summarizeResult(testResults, build, pipelineTestDetails, workspace, launcher, listener, storage);
}

Expand Down Expand Up @@ -498,6 +500,11 @@ public boolean isSkipPublishingChecks() {
return skipPublishingChecks;
}

@DataBoundSetter
public final void setAllowEmptyResults(boolean allowEmptyResults) {
this.allowEmptyResults = allowEmptyResults;
}

@DataBoundSetter
public void setSkipPublishingChecks(boolean skipPublishingChecks) {
this.skipPublishingChecks = skipPublishingChecks;
Expand All @@ -513,11 +520,6 @@ public void setChecksName(String checksName) {
this.checksName = checksName;
}

@DataBoundSetter
public final void setAllowEmptyResults(boolean allowEmptyResults) {
this.allowEmptyResults = allowEmptyResults;
}

public boolean isSkipMarkingBuildUnstable() {
return skipMarkingBuildUnstable;
}
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Return the appropriate value or a default (e.g., false)

return sortTestResultsByTimestamp;
}

// Add the field and setter if needed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Add the field and setter if needed

private boolean sortTestResultsByTimestamp;

@DataBoundSetter
public void setSortTestResultsByTimestamp(boolean sortTestResultsByTimestamp) {
this.sortTestResultsByTimestamp = sortTestResultsByTimestamp;
}

@Extension
public static class DescriptorImpl extends BuildStepDescriptor<Publisher> {
@Override
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/hudson/tasks/junit/JUnitTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,6 @@ default StdioRetention getParsedStdioRetention() {
String getChecksName();

boolean isSkipOldReports();

boolean isSortTestResultsByTimestamp();
}
12 changes: 12 additions & 0 deletions src/main/java/hudson/tasks/junit/pipeline/JUnitResultsStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@

private Double healthScaleFactor;

private boolean sortTestResultsByTimestamp;

/**
* If true, don't throw exception on missing test results or no files found.
*/
Expand Down Expand Up @@ -94,6 +96,16 @@
this.healthScaleFactor = Math.max(0.0, healthScaleFactor);
}

@Override
public boolean isSortTestResultsByTimestamp() {
return sortTestResultsByTimestamp;
}

@DataBoundSetter
public void setSortTestResultsByTimestamp(boolean sortTestResultsByTimestamp) {
this.sortTestResultsByTimestamp = sortTestResultsByTimestamp;
}

Check warning on line 107 in src/main/java/hudson/tasks/junit/pipeline/JUnitResultsStep.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 106-107 are not covered by tests

@NonNull
@Override
public List<TestDataPublisher> getTestDataPublishers() {
Expand Down
3 changes: 3 additions & 0 deletions src/main/resources/hudson/tasks/junit/JUnitResultArchiver/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,7 @@ THE SOFTWARE.
<f:entry title="${%Skip report files older than build start}" field="skipOldReports">
<f:checkbox default="false" title="${%If checked, the test report files older than build start will not be included in the parsing}"/>
</f:entry>
<f:entry field="sortTestResultsByTimestamp" title="Sort test result files by modification timestamp">
<f:checkbox/>
</f:entry>
</j:jelly>
20 changes: 15 additions & 5 deletions src/test/java/hudson/tasks/junit/JUnitResultArchiverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@
import hudson.tasks.test.helper.WebClientFactory;
import hudson.util.HttpResponses;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.URL;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -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");
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes related to your change? You can submit them separately with reasoning if not

if (xxeResourceUrl == null) {
throw new FileNotFoundException("Resource 'testXxe-xxe.xml' not found");
}
File xxeFile = new File(xxeResourceUrl.toURI());
String xxeFileContent = FileUtils.readFileToString(xxeFile, StandardCharsets.UTF_8);
String adaptedXxeFileContent = xxeFileContent.replace("$OOB_LINK$", oobInUserContentLink);

String oobFile = this.getClass().getResource("testXxe-oob.xml").getFile();
String oobFileContent = FileUtils.readFileToString(new File(oobFile), StandardCharsets.UTF_8);
URL oobResourceUrl = this.getClass().getResource("testXxe-oob.xml");
if (oobResourceUrl == null) {
throw new FileNotFoundException("Resource 'testXxe-oob.xml' not found");
}
File oobFile = new File(oobResourceUrl.toURI());
String oobFileContent = FileUtils.readFileToString(oobFile, StandardCharsets.UTF_8);
String adaptedOobFileContent = oobFileContent.replace("$TARGET_URL$", triggerLink);

File userContentDir = new File(j.jenkins.getRootDir(), "userContent");
FileUtils.writeStringToFile(new File(userContentDir, "oob.xml"), adaptedOobFileContent);
FileUtils.writeStringToFile(new File(userContentDir, "oob.xml"), adaptedOobFileContent, StandardCharsets.UTF_8);

FreeStyleProject project = j.createFreeStyleProject();
DownloadBuilder builder = new DownloadBuilder();
Expand Down
Loading