Skip to content

findbugs: fix issues in BuildData #44

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

Merged
jakub-bochenski merged 3 commits into from
Dec 29, 2017
Merged

findbugs: fix issues in BuildData #44

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b4c6d7
Merge pull request #1 from jenkinsci/master
mwinter69 Dec 27, 2017
ca4eca8
fix findbugs issues in buildData
mwinter69 Dec 28, 2017
68491f6
keep date format as it was before
mwinter69 Dec 29, 2017
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
44 changes: 21 additions & 23 deletions src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,15 @@

import hudson.model.Action;
import hudson.model.Environment;
import hudson.model.Executor;
import hudson.model.Result;
import hudson.model.AbstractBuild;
import hudson.model.TaskListener;
import hudson.model.Run;
import hudson.model.Node;
import hudson.model.Executor;
import hudson.tasks.test.AbstractTestResultAction;
import hudson.tasks.test.TestResult;

import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.Collections;
Expand All @@ -53,6 +51,7 @@
import net.sf.json.JSONObject;

import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.time.FastDateFormat;

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
Expand All @@ -65,7 +64,7 @@
*/
public class BuildData {
// ISO 8601 date format
public transient static final DateFormat DATE_FORMATTER = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ");
public transient static final FastDateFormat DATE_FORMATTER = FastDateFormat.getInstance("yyyy-MM-dd'T'HH:mm:ss.SSSZ");
Copy link

@jakub-bochenski jakub-bochenski Dec 29, 2017

Choose a reason for hiding this comment

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

I'm not sure.. the addition of milliseconds can be a breaking change if somebody is parsing the date with a strict parser. I'd suggest extracting the format to configuration if you need the milis part

Copy link
Author

@mwinter69 mwinter69 Dec 29, 2017

Choose a reason for hiding this comment

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

ok, I will leave the format as is for now. Making this configurable could be done with the refactoring of configuration

Copy link

@jakub-bochenski jakub-bochenski Dec 29, 2017

Choose a reason for hiding this comment

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

While we'r at it can you restrict it's visibility? The formatter has been made private in #40

Copy link
Author

@mwinter69 mwinter69 Dec 29, 2017

Choose a reason for hiding this comment

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

I would like to keep changes in this PR to the findbugs issues. providing a method to access the formatter should be done in a separate PR.

private final static Logger LOGGER = Logger.getLogger(MethodHandles.lookup().lookupClass().getCanonicalName());
public static class TestData {
int totalCount, skipCount, failCount, passCount;
Expand Down Expand Up @@ -138,15 +137,6 @@ public TestData(Action action) {
public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener) {
initData(build, currentTime);

Node node = build.getExecutor().getOwner().getNode();
if (node == null) {
buildHost = "master";
buildLabel = "master";
} else {
buildHost = StringUtils.isBlank(node.getDisplayName()) ? "master" : node.getDisplayName();
buildLabel = StringUtils.isBlank(node.getLabelString()) ? "master" : node.getLabelString();
}

// build.getDuration() is always 0 in Notifiers
rootProjectName = build.getRootBuild().getProject().getName();
rootFullProjectName = build.getRootBuild().getProject().getFullName();
Expand Down Expand Up @@ -186,15 +176,6 @@ public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener liste
public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
initData(build, currentTime);

Node node = build.getExecutor().getOwner().getNode();
if (node == null) {
buildHost = "master";
buildLabel = "master";
} else {
buildHost = StringUtils.isBlank(node.getDisplayName()) ? "master" : node.getDisplayName();
buildLabel = StringUtils.isBlank(node.getLabelString()) ? "master" : node.getLabelString();
}

rootProjectName = projectName;
rootFullProjectName = fullProjectName;
rootProjectDisplayName = displayName;
Expand All @@ -210,7 +191,24 @@ public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
}

private void initData(Run<?, ?> build, Date currentTime) {
result = build.getResult() == null ? null : build.getResult().toString();

Executor executor = build.getExecutor();
if (executor == null) {
buildHost = "master";
buildLabel = "master";
} else {
Node node = executor.getOwner().getNode();
if (node == null) {
buildHost = "master";
buildLabel = "master";
} else {
buildHost = StringUtils.isBlank(node.getDisplayName()) ? "master" : node.getDisplayName();
buildLabel = StringUtils.isBlank(node.getLabelString()) ? "master" : node.getLabelString();
}
}

Result result = build.getResult();
this.result = result == null ? null : result.toString();
id = build.getId();
projectName = build.getParent().getName();
fullProjectName = build.getParent().getFullName();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/jenkins/plugins/logstash/LogstashWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ public void constructorSuccess() throws Exception {
// Verify that the BuildData constructor is what is being called here.
// This also lets us verify that in the instantiation failure cases we do not construct BuildData.
verify(mockBuild).getId();
verify(mockBuild, times(2)).getResult();
verify(mockBuild).getResult();
verify(mockBuild, times(2)).getParent();
verify(mockBuild, times(2)).getProject();
verify(mockBuild, times(1)).getStartTimeInMillis();
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/jenkins/plugins/logstash/persistence/BuildDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ private void verifyMocks() throws Exception
verify(mockProject).getFullName();

verify(mockBuild).getId();
verify(mockBuild, times(2)).getResult();
verify(mockBuild).getResult();
verify(mockBuild, times(2)).getParent();
verify(mockBuild).getDisplayName();
verify(mockBuild).getFullDisplayName();
Expand Down