Skip to content

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

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

Open
blackapple805 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 @@ public class JUnitParser extends TestResultParser {
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,7 +76,7 @@ public JUnitParser() {
*/
@Deprecated
public JUnitParser(boolean keepLongStdio) {
this(keepLongStdio, false, false, false);
this(StdioRetention.fromKeepLongStdio(keepLongStdio), false, false, false, false);
}

/**
Expand All @@ -81,32 +86,50 @@ public JUnitParser(boolean keepLongStdio) {
*/
@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);
}
// New Constructor with the additional parameter
Copy link
Member

@timja timja Nov 25, 2024

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 @@ public TestResult parseResult(
keepTestNames,
pipelineTestDetails,
listener,
skipOldReports));
skipOldReports,
sortTestResultsByTimestamp));
}

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

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

private boolean skipOldReports;
private final boolean sortTestResultsByTimestamp;

private ParseResultCallable(
String testResults,
Expand All @@ -204,7 +230,8 @@ private ParseResultCallable(
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 @@ private ParseResultCallable(
this.pipelineTestDetails = pipelineTestDetails;
this.listener = listener;
this.skipOldReports = skipOldReports;
this.sortTestResultsByTimestamp = sortTestResultsByTimestamp;
}

@Override
Expand All @@ -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
Copy link
Member

@timja timja Nov 25, 2024

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

@timja timja Nov 25, 2024

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) {
Collections.sort(fileList, Comparator.comparingLong(File::lastModified));
} 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)) {
sortedFiles[i] = absolutePath.substring(baseDirPath.length() + 1);
} else {
sortedFiles[i] = absolutePath;
}
}

// 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 @@ private static final class DirectParseResultCallable extends ParseResultCallable
boolean keepTestNames,
PipelineTestDetails pipelineTestDetails,
TaskListener listener,
boolean skipOldReports) {
boolean skipOldReports,
boolean sortTestResultsByTimestamp) {
super(
testResults,
build,
Expand All @@ -280,7 +337,8 @@ private static final class DirectParseResultCallable extends ParseResultCallable
keepTestNames,
pipelineTestDetails,
listener,
skipOldReports);
skipOldReports,
sortTestResultsByTimestamp);
}

@Override
Expand All @@ -303,7 +361,8 @@ private static final class StorageParseResultCallable extends ParseResultCallabl
PipelineTestDetails pipelineTestDetails,
TaskListener listener,
JunitTestResultStorage.RemotePublisher publisher,
boolean skipOldReports) {
boolean skipOldReports,
boolean sortTestResultsByTimestamp) {
super(
testResults,
build,
Expand All @@ -313,7 +372,8 @@ private static final class StorageParseResultCallable extends ParseResultCallabl
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

@timja timja Nov 25, 2024

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

@timja timja Nov 25, 2024

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 @@ public class JUnitResultsStep extends Step implements JUnitTask {

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 @@ public final void setHealthScaleFactor(double healthScaleFactor) {
this.healthScaleFactor = Math.max(0.0, healthScaleFactor);
}

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

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

@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

@timja timja Nov 25, 2024

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