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

[#1943] Make ErrorSummary non-static by removing singleton design pattern #2021

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion docs/dg/learningBasics.md
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ Here are some small tasks for you to gain some basic knowledge of the code relat
Add this to the catch block of `spawnCloneProcess` and `waitForCloneProcess`, so that the message will be captured in `summary.json`.

```
ErrorSummary.getInstance().addErrorMessage(config.getDisplayName(), e.getMessage());
new ErrorSummary().addErrorMessage(config.getDisplayName(), e.getMessage());
```
</panel>

Expand Down
12 changes: 9 additions & 3 deletions src/main/java/reposense/model/RepoLocation.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ public class RepoLocation {
private final transient String outputFolderRepoName;
private final transient String outputFolderOrganization;

private final ErrorSummary errorSummary = new ErrorSummary();

/**
* Creates {@link RepoLocation} based on the {@code location}, which is represented by a {@code URL}
* or {@link Path}.
Expand Down Expand Up @@ -110,6 +112,10 @@ public String getDomainName() {
return domainName;
}

public ErrorSummary getErrorSummary() {
return errorSummary;
}

/**
* Returns true if {@code repoArgument} is a valid local repository argument.
* This implementation follows directly from the {@code git clone}
Expand Down Expand Up @@ -159,7 +165,7 @@ private String[] getLocalRepoNameAndOrg(String location) throws InvalidLocationE
Matcher localRepoMatcher = localRepoPattern.matcher(location);

if (!localRepoMatcher.matches()) {
ErrorSummary.getInstance().addErrorMessage(location,
errorSummary.addErrorMessage(location,
String.format(MESSAGE_INVALID_LOCATION, location));
throw new InvalidLocationException(String.format(MESSAGE_INVALID_LOCATION, location));
}
Expand All @@ -185,14 +191,14 @@ private String[] getRemoteRepoNameAndOrg(String location) throws InvalidLocation
try {
new URI(location);
} catch (URISyntaxException e) {
ErrorSummary.getInstance().addErrorMessage(location,
errorSummary.addErrorMessage(location,
String.format(MESSAGE_INVALID_REMOTE_URL, location));
throw new InvalidLocationException(String.format(MESSAGE_INVALID_REMOTE_URL, location));
}
}
boolean isValidRemoteRepoUrl = remoteRepoMatcher.matches() || sshRepoMatcher.matches();
if (!isValidRemoteRepoUrl) {
ErrorSummary.getInstance().addErrorMessage(location,
errorSummary.addErrorMessage(location,
String.format(MESSAGE_INVALID_REMOTE_URL, location));
throw new InvalidLocationException(String.format(MESSAGE_INVALID_REMOTE_URL, location));
}
Expand Down
10 changes: 1 addition & 9 deletions src/main/java/reposense/report/ErrorSummary.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,7 @@
* Holds the data of set of repos that failed to analyze and the reasons for the failed operation.
*/
public class ErrorSummary {
private static ErrorSummary instance = null;
private static Set<Map<String, String>> errorSet = new HashSet<>();

public static ErrorSummary getInstance() {
if (instance == null) {
instance = new ErrorSummary();
}
return instance;
}
private Set<Map<String, String>> errorSet = new HashSet<>();

/**
* Adds an error message for {@code repoName} with the reason {@code errorMessage} into a set of errors.
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/reposense/report/ReportGenerator.java
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ public class ReportGenerator {
private LocalDateTime earliestSinceDate = null;
private ProgressTracker progressTracker = null;

private ErrorSummary errorSummary = new ErrorSummary();

/**
* Generates the authorship and commits JSON file for each repo in {@code configs} at {@code outputPath}, as
* well as the summary JSON file of all the repos.
Expand Down Expand Up @@ -135,7 +137,7 @@ public List<Path> generateReposReport(List<RepoConfiguration> configs, String ou
Optional<Path> summaryPath = FileUtil.writeJsonFile(
new SummaryJson(configs, reportConfig, generationDate,
reportSinceDate, untilDate, isSinceDateProvided,
isUntilDateProvided, RepoSense.getVersion(), ErrorSummary.getInstance().getErrorSet(),
isUntilDateProvided, RepoSense.getVersion(), errorSummary.getErrorSet(),
reportGenerationTimeProvider.get(), zoneId),
getSummaryResultPath(outputPath));
summaryPath.ifPresent(reportFoldersAndFiles::add);
Expand Down Expand Up @@ -456,7 +458,7 @@ private void handleFailedConfigs(List<RepoConfiguration> configs, List<RepoConfi
while (itr.hasNext()) {
RepoConfiguration config = itr.next();
if (failedConfigs.contains(config)) {
ErrorSummary.getInstance().addErrorMessage(config.getDisplayName(), errorMessage);
errorSummary.addErrorMessage(config.getDisplayName(), errorMessage);
itr.remove();
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/systemtest/java/reposense/ConfigSystemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import reposense.git.GitVersion;
import reposense.model.SupportedDomainUrlMap;
import reposense.parser.SinceDateArgumentType;
import reposense.report.ErrorSummary;
import reposense.util.FileUtil;
import reposense.util.InputBuilder;
import reposense.util.SystemTestUtil;
Expand All @@ -39,7 +38,6 @@ public class ConfigSystemTest {
public void setUp() throws Exception {
SupportedDomainUrlMap.clearAccessedSet();
FileUtil.deleteDirectory(OUTPUT_DIRECTORY);
ErrorSummary.getInstance().clearErrorSet();
}

@AfterEach
Expand Down
2 changes: 0 additions & 2 deletions src/systemtest/java/reposense/LocalRepoSystemTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import reposense.model.RepoLocation;
import reposense.model.SupportedDomainUrlMap;
import reposense.parser.SinceDateArgumentType;
import reposense.report.ErrorSummary;
import reposense.util.FileUtil;
import reposense.util.InputBuilder;
import reposense.util.SystemTestUtil;
Expand Down Expand Up @@ -49,7 +48,6 @@ public static void setupLocalRepos() throws Exception {
public void setupLocalTest() throws Exception {
SupportedDomainUrlMap.clearAccessedSet();
FileUtil.deleteDirectory(OUTPUT_DIRECTORY);
ErrorSummary.getInstance().clearErrorSet();
}

@AfterEach
Expand Down
24 changes: 10 additions & 14 deletions src/test/java/reposense/report/ErrorSummaryTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,43 +12,39 @@ public void errorSummary_addRepeatedErrorMessage_containsNoDuplicates() {
String invalidLocation1 = "ttp://github.com/reposense.RepoSense.git";
String invalidLocation2 = "https://github.com/contains-illegal-chars/^\\/";
String invalidLocation3 = "not-valid-protocol://abc.com/reposense/RepoSense.git";

ErrorSummary errorSummaryInstance = ErrorSummary.getInstance();
errorSummaryInstance.clearErrorSet();

try {
new RepoLocation(invalidLocation1);
RepoLocation repoLocation = new RepoLocation(invalidLocation1);
Assertions.assertEquals(1, repoLocation.getErrorSummary().getErrorSet().size());
} catch (InvalidLocationException e) {
// not relevant to the test
}
Assertions.assertEquals(1, errorSummaryInstance.getErrorSet().size());

try {
new RepoLocation(invalidLocation1);
RepoLocation repoLocation = new RepoLocation(invalidLocation1);
Assertions.assertEquals(1, repoLocation.getErrorSummary().getErrorSet().size());
} catch (InvalidLocationException e) {
// not relevant to the test
}
Assertions.assertEquals(1, errorSummaryInstance.getErrorSet().size());

try {
new RepoLocation(invalidLocation2);
RepoLocation repoLocation = new RepoLocation(invalidLocation2);
Assertions.assertEquals(2, repoLocation.getErrorSummary().getErrorSet().size());
} catch (InvalidLocationException e) {
// not relevant to the test
}
Assertions.assertEquals(2, errorSummaryInstance.getErrorSet().size());

try {
new RepoLocation(invalidLocation1);
RepoLocation repoLocation = new RepoLocation(invalidLocation1);
Assertions.assertEquals(2, repoLocation.getErrorSummary().getErrorSet().size());
} catch (InvalidLocationException e) {
// not relevant to the test
}
Assertions.assertEquals(2, errorSummaryInstance.getErrorSet().size());

try {
new RepoLocation(invalidLocation3);
RepoLocation repoLocation = new RepoLocation(invalidLocation3);
Assertions.assertEquals(3, repoLocation.getErrorSummary().getErrorSet().size());
} catch (InvalidLocationException e) {
// not relevant to the test
}
Assertions.assertEquals(3, errorSummaryInstance.getErrorSet().size());
}
}
Loading