Skip to content

Commit 534afa0

Browse files
theKBrotheKBro
authored
- invoke log string building methods only conditionally (#178)
- mark overriden methods deprecated when base is deprecated - use java.nio.file.Files#delete instead of file#delete and adapt tests accordingly using static mocking
1 parent 8fcdf8d commit 534afa0

File tree

6 files changed

+73
-71
lines changed

6 files changed

+73
-71
lines changed

‎src/main/java/pl/damianszczepanik/jenkins/buildhistorymanager/BuildHistoryManager.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.io.IOException;
44
import java.util.List;
5+
import java.util.logging.Level;
56
import java.util.logging.Logger;
67

78
import hudson.Util;
@@ -86,6 +87,6 @@ private void processRules(Run<?, ?> run, String uniquePerformName) throws IOExce
8687
}
8788

8889
private static void log(String jobName, String message) {
89-
LOG.fine(String.format("[%s] %s", jobName, message));
90+
LOG.log(Level.FINE, () -> String.format("[%s] %s", jobName, message));
9091
}
9192
}

‎src/main/java/pl/damianszczepanik/jenkins/buildhistorymanager/model/Rule.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.io.IOException;
44
import java.util.List;
5+
import java.util.logging.Level;
56
import java.util.logging.Logger;
67

78
import hudson.Util;
@@ -106,6 +107,6 @@ public void performActions(Run<?, ?> run) throws IOException, InterruptedExcepti
106107
}
107108

108109
private static void log(String jobName, String message) {
109-
LOG.fine(String.format("[%s] %s", jobName, message));
110+
LOG.log(Level.FINE, () -> String.format("[%s] %s", jobName, message));
110111
}
111112
}

‎src/main/java/pl/damianszczepanik/jenkins/buildhistorymanager/model/actions/DeleteLogFileAction.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
import java.io.File;
44
import java.io.IOException;
5+
import java.nio.file.Files;
6+
import java.util.logging.Level;
57
import java.util.logging.Logger;
68

79
import hudson.model.Run;
@@ -33,13 +35,11 @@ public void perform(Run<?, ?> run) throws IOException, InterruptedException {
3335
return;
3436
}
3537
if (logFile.exists()) {
36-
if(!logFile.delete()) {
37-
log(run.getParent().getFullDisplayName(), "Cannot delete log file");
38-
}
38+
Files.delete(logFile.toPath());
3939
}
4040
}
4141

4242
private static void log(String jobName, String message) {
43-
LOG.fine(String.format("[%s] %s", jobName, message));
43+
LOG.log(Level.FINE, () -> String.format("[%s] %s", jobName, message));
4444
}
4545
}

‎src/main/java/pl/damianszczepanik/jenkins/buildhistorymanager/model/conditions/TokenMacroCondition.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.io.File;
44
import java.io.IOException;
5+
import java.util.logging.Level;
56
import java.util.logging.Logger;
67

78
import hudson.FilePath;
@@ -53,11 +54,11 @@ public boolean matches(Run<?, ?> run, RuleConfiguration configuration) {
5354
try {
5455
File workspace = run.getRootDir();
5556
String evaluatedMacro = TokenMacro.expandAll(run, new FilePath(workspace), null, template);
56-
LOG.info(String.format("Evaluated macro '%s' to '%s'", template, evaluatedMacro));
57+
LOG.log(Level.INFO, () -> String.format("Evaluated macro '%s' to '%s'", template, evaluatedMacro));
5758
return StringUtils.defaultString(value).equals(evaluatedMacro);
5859

5960
} catch (InterruptedException | IOException | MacroEvaluationException e) {
60-
LOG.warning(String.format("Exception when processing template '%s' for build #%d: %s",
61+
LOG.log(Level.WARNING, () ->String.format("Exception when processing template '%s' for build #%d: %s",
6162
template, run.getNumber(), e.getMessage()));
6263
return false;
6364
}

‎src/test/java/pl/damianszczepanik/jenkins/buildhistorymanager/model/actions/DeleteLogFileActionTest.java

+54-33
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,20 @@
11
package pl.damianszczepanik.jenkins.buildhistorymanager.model.actions;
22

3+
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.junit.jupiter.api.Assertions.assertThrows;
35
import static org.mockito.Mockito.mock;
6+
import static org.mockito.Mockito.mockStatic;
7+
import static org.mockito.Mockito.times;
48
import static org.mockito.Mockito.when;
59

610
import java.io.File;
711
import java.io.IOException;
12+
import java.nio.file.Files;
13+
import java.nio.file.Path;
814

15+
import edu.umd.cs.findbugs.annotations.NonNull;
916
import org.junit.jupiter.api.Test;
17+
import org.mockito.MockedStatic;
1018
import pl.damianszczepanik.jenkins.buildhistorymanager.utils.RunStub;
1119

1220
/**
@@ -16,40 +24,56 @@ class DeleteLogFileActionTest {
1624

1725
@Test
1826
void perform_OnExistingLogFile_DeletesLogFile() throws IOException, InterruptedException {
19-
2027
// given
28+
File logFile = mock(File.class);
29+
Path logFilePath = mock(Path.class);
30+
when(logFile.exists()).thenReturn(true);
31+
when(logFile.toPath()).thenReturn(logFilePath);
32+
2133
Action action = new DeleteLogFileAction();
22-
RunStub run = new RunStub(RunStub.LogFileAvailability.PRESENT);
23-
24-
// when
25-
action.perform(run);
26-
27-
// then
28-
run.assertLogFileIsNotAvailable();
34+
RunStub run = new RunStub(logFile);
35+
36+
37+
try (MockedStatic<Files> mockFiles = mockStatic(Files.class)) {
38+
// when
39+
action.perform(run);
40+
// then
41+
mockFiles.verify(() -> Files.delete(logFilePath), times(1));
42+
}
2943
}
3044

3145
@Test
3246
void perform_OnMissingLogFile_SkipDeletion() throws IOException, InterruptedException {
3347

3448
// given
49+
File logFile = mock(File.class);
50+
Path logFilePath = mock(Path.class);
51+
when(logFile.exists()).thenReturn(false);
52+
when(logFile.toPath()).thenReturn(logFilePath);
53+
3554
Action action = new DeleteLogFileAction();
36-
RunStub run = new RunStub(RunStub.LogFileAvailability.ABSENT);
55+
RunStub run = new RunStub(logFile);
3756

38-
// when
39-
action.perform(run);
57+
try (MockedStatic<Files> mockFiles = mockStatic(Files.class)) {
58+
// when
59+
action.perform(run);
60+
// then
61+
mockFiles.verify(() -> Files.delete(logFilePath), times(0));
62+
}
4063

41-
// then
42-
run.assertLogFileIsNotAvailable();
4364
}
4465

4566
@Test
4667
void perform_OnGetLogFileThrowsUnsupportedOperationException_CatchesExceptionAndDoesNothing() throws IOException, InterruptedException {
47-
4868
// given
69+
final boolean[] exceptionWasThrown = {false};
4970
Action action = new DeleteLogFileAction();
50-
RunStub run = new RunStub(RunStub.LogFileAvailability.PRESENT) {
71+
RunStub run = new RunStub() {
72+
@NonNull
5173
@Override
74+
@Deprecated
5275
public File getLogFile() {
76+
exceptionWasThrown[0] = true;
5377
throw new UnsupportedOperationException("Operation not supported");
5478
}
5579
};
@@ -58,29 +82,26 @@ public File getLogFile() {
5882
action.perform(run);
5983

6084
// then
61-
run.assertLogFileIsAvailable();
85+
assertThat(exceptionWasThrown[0]).isTrue();
6286
}
6387

6488
@Test
65-
void perform_DeleteLogFileDoesNotWork_IgnoresFailure() throws IOException, InterruptedException {
66-
89+
void perform_DeleteLogFileDoesNotWork_Throws() throws IOException {
6790
// given
91+
Path logFilePath = mock(Path.class);
92+
File logFile = mock(File.class);
93+
when(logFile.exists()).thenReturn(true);
94+
when(logFile.toPath()).thenReturn(logFilePath);
95+
6896
Action action = new DeleteLogFileAction();
69-
RunStub run = new RunStub(RunStub.LogFileAvailability.PRESENT) {
70-
@Override
71-
public File getLogFile() {
72-
File logFile = mock(File.class);
73-
when(logFile.exists()).thenReturn(true);
74-
when(logFile.delete()).thenReturn(false);
75-
return logFile;
76-
}
77-
};
78-
79-
// when
80-
action.perform(run);
81-
82-
// then
83-
run.assertLogFileIsAvailable();
97+
RunStub run = new RunStub(logFile);
98+
99+
try (MockedStatic<Files> mockFiles = mockStatic(Files.class)) {
100+
mockFiles.when(() -> Files.delete(logFilePath)).thenThrow(new IOException("File deletion failed"));
101+
102+
// when / then
103+
assertThrows(IOException.class, () -> action.perform(run));
104+
}
84105
}
85106

86107
}

‎src/test/java/pl/damianszczepanik/jenkins/buildhistorymanager/utils/RunStub.java

+8-30
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.assertj.core.api.Assertions.assertThat;
44
import static org.mockito.Mockito.mock;
5-
import static org.mockito.Mockito.when;
65

76
import java.io.File;
87
import java.io.IOException;
@@ -11,6 +10,7 @@
1110
import java.util.Date;
1211
import java.util.List;
1312

13+
import edu.umd.cs.findbugs.annotations.NonNull;
1414
import hudson.model.Cause;
1515
import hudson.model.Job;
1616
import hudson.model.Result;
@@ -23,20 +23,16 @@
2323
* @author Damian Szczepanik (damianszczepanik@github)
2424
*/
2525
public class RunStub extends Run {
26-
public enum LogFileAvailability {
27-
PRESENT, ABSENT
28-
}
29-
3026
private Result result;
31-
private LogFileAvailability logFileState = LogFileAvailability.PRESENT;
27+
private File logFile = null;
3228
private List<Cause> causes;
3329

3430
private int deleteArtifactsTimes;
3531
private int deleteTimes;
3632

37-
public RunStub(LogFileAvailability logFileState) throws IOException {
33+
public RunStub(File logFile) throws IOException {
3834
this();
39-
this.logFileState = logFileState;
35+
this.logFile = logFile;
4036
}
4137

4238
public RunStub(int buildNumber) throws IOException {
@@ -101,12 +97,9 @@ public void delete() {
10197
}
10298

10399
@Override
104-
public File getLogFile() {
105-
File logFile = mock(File.class);
106-
when(logFile.exists()).thenReturn(logFileState == LogFileAvailability.PRESENT);
107-
when(logFile.delete()).thenReturn(deleteLogFile());
108-
109-
return logFile;
100+
@Deprecated
101+
public @NonNull File getLogFile() {
102+
return logFile != null ? logFile : mock(File.class);
110103
}
111104

112105
// skips serialization which is quite problematic
@@ -119,12 +112,6 @@ public synchronized void save() {
119112
public void checkPermission(Permission permission) {
120113
}
121114

122-
private boolean deleteLogFile() {
123-
boolean deleteResult = logFileState == LogFileAvailability.PRESENT;
124-
logFileState = LogFileAvailability.ABSENT;
125-
return deleteResult;
126-
}
127-
128115
public void assertBuildWasDeleted() {
129116
assertThat(deleteTimes).isOne();
130117
}
@@ -141,15 +128,6 @@ public void assertArtifactsAreAvailable() {
141128
assertThat(deleteArtifactsTimes).isZero();
142129
}
143130

144-
public void assertLogFileIsNotAvailable() {
145-
assertThat(logFileState).isEqualTo(LogFileAvailability.ABSENT);
146-
}
147-
148-
public void assertLogFileIsAvailable() {
149-
assertThat(logFileState).isEqualTo(LogFileAvailability.PRESENT);
150-
}
151-
152-
153131
@Override
154132
public Result getResult() {
155133
return result;
@@ -161,7 +139,7 @@ public long getDuration() {
161139
}
162140

163141
@Override
164-
public List<Cause> getCauses() {
142+
public @NonNull List<Cause> getCauses() {
165143
return causes;
166144
}
167145

0 commit comments

Comments
 (0)