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

Fix spotbug findings #68

Merged
merged 7 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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: 2 additions & 0 deletions Jenkinsfile.internal
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ pipeline {
failFast false
parallel {
stage('System Tests') {
// The needed artifact is only built for main and pull requests -> skip this job otherwise
when { branch pattern: "PR-\\d+|main", comparator: "REGEXP"}
steps {
build job: '../systemTests', parameters: [string(name: 'artifactName', value: "${JOB_BASE_NAME}"), string(name: 'commitHash', value: "${GIT_COMMIT}")]
}
Expand Down
17 changes: 17 additions & 0 deletions config/spotbugs/spotbugs-exclude.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,21 @@
<Match>
<Source name="~.*\.java" />
</Match>
<Match>
<Class name="~.*_closure\d+" />
<Bug pattern="SE_NO_SERIALVERSIONID" />
</Match>
<Match>
<Field name="metaClass" />
<Bug code="EI,EI2"/>
</Match>
<Match>
<Class name="~.*_closure\d+" />
<Bug code="EI2" />
</Match>
<Match>
<Class name="~.*AbstractTestBuilder" />
<Field name="context" />
<Bug pattern="EI_EXPOSE_REP" />
</Match>
</FindBugsFilter>
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ETInstallation extends ToolInstallation implements
* @param home path to the tool executable
* @param properties properties of the tool installation
*/
ETInstallation(String name, String home, List<? extends ToolProperty<?>> properties) {
ETInstallation(String name, String home, List<? extends ToolProperty<? extends ToolInstallation>> properties) {
super(Util.fixEmptyAndTrim(name), Util.fixEmptyAndTrim(home), properties)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ class RunPackageAction extends RunTestAction {
}

PackageConfig getPackageConfig() {
return packageConfig
return new PackageConfig(packageConfig)
}

AnalysisConfig getAnalysisConfig() {
return analysisConfig
return new AnalysisConfig(analysisConfig)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ abstract class RunTestAction extends InvisibleAction {
}

TestConfig getTestConfig() {
return testConfig
return new TestConfig(testConfig)
}

TestResult getTestResult() {
return testResult
return new TestResult(testResult)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ import java.util.concurrent.TimeoutException
* Common base class for all test related steps implemented in this plugin.
*/
abstract class AbstractTestBuilder implements Serializable {
static String testCasePath
static TestConfig testConfig
static ExecutionConfig executionConfig
static StepContext context
final String testCasePath
final TestConfig testConfig
final ExecutionConfig executionConfig
final StepContext context

protected abstract String getTestArtifactName()
protected abstract LogConfigUtil getLogConfig()
Expand All @@ -43,6 +43,14 @@ abstract class AbstractTestBuilder implements Serializable {
this.context = context
}

ExecutionConfig getExecutionConfig() {
return executionConfig? new ExecutionConfig(executionConfig) : null
Copy link
Contributor

Choose a reason for hiding this comment

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

format -> return executionConfig ? new ExecutionConfig(executionConfig) : null
and L51

}

TestConfig getTestConfig() {
return testConfig? new TestConfig(testConfig) : null
}

TestResult runTest() {

def toolInstallations = getToolInstallationsOnNode()
Expand All @@ -52,7 +60,7 @@ abstract class AbstractTestBuilder implements Serializable {
getTestArtifactName(), getLogConfig(), getExecutionOrderBuilder(), toolInstallations))
}

private static ArrayList<String> getToolInstallationsOnNode() {
private ArrayList<String> getToolInstallationsOnNode() {
/**
* This method gets the executable names of the tool installations on the node given by the context. Context is
* not reasonably available in the MasterToSlaveCallable, so all info which needs a context must be fetched
Expand All @@ -73,6 +81,9 @@ abstract class AbstractTestBuilder implements Serializable {
}

private static final class RunTestCallable extends MasterToSlaveCallable<TestResult, IOException> {

private static final long serialVersionUID = 1L

private final String testCasePath
private final EnvVars envVars
private final TaskListener listener
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import de.tracetronic.jenkins.plugins.ecutestexecution.util.ConverterUtil
* thus avoid serialization errors.
*/
class ExecutionOrderBuilder implements Serializable {

private static final long serialVersionUID = 1L

private final String testCasePath
private final TestConfig testConfig
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@ import org.jenkinsci.plugins.workflow.steps.StepContext
* builder providing test package configuration.
*/
class TestPackageBuilder extends AbstractTestBuilder {
private static final long serialVersionUID = 1L

/**
* Defines the test artifact name.
*/
private final static String TEST_ARTIFACT_NAME = 'package'
private static PackageConfig packageConfig
private static AnalysisConfig analysisConfig
private final PackageConfig packageConfig
private final AnalysisConfig analysisConfig

TestPackageBuilder(String testCasePath, TestConfig testConfig, ExecutionConfig executionConfig,
StepContext context, PackageConfig packageConfig, AnalysisConfig analysisConfig){
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import org.jenkinsci.plugins.workflow.steps.StepContext
* builder providing test project configuration.
*/
class TestProjectBuilder extends AbstractTestBuilder {

private static final long serialVersionUID = 1L

/**
* Defines the test artifact name.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ class AnalysisConfig extends AbstractDescribableImpl<AnalysisConfig> implements
this.recordings = []
}

AnalysisConfig(AnalysisConfig config) {
this.analysisName = config.getAnalysisName()
this.mapping = config.getMapping()
this.recordings = config.getRecordings()
}

String getAnalysisName() {
return analysisName
}
Expand All @@ -52,7 +58,7 @@ class AnalysisConfig extends AbstractDescribableImpl<AnalysisConfig> implements
}

List<RecordingAsSetting> getRecordings() {
return recordings
return recordings.collect({new RecordingAsSetting(it)})
}

@DataBoundSetter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import hudson.Extension
import hudson.model.AbstractDescribableImpl
import hudson.model.Descriptor
import hudson.util.FormValidation
import org.apache.tools.ant.taskdefs.Exec
Copy link
Contributor

Choose a reason for hiding this comment

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

probably unused

import org.kohsuke.stapler.DataBoundConstructor
import org.kohsuke.stapler.DataBoundSetter
import org.kohsuke.stapler.QueryParameter
Expand All @@ -32,6 +33,12 @@ class ExecutionConfig extends AbstractDescribableImpl<ExecutionConfig> implement
this.stopUndefinedTools = true
}

ExecutionConfig(ExecutionConfig config) {
this.timeout = config.getTimeout()
this.stopOnError = config.isStopOnError()
this.stopUndefinedTools = config.getStopUndefinedTools()
}

int getTimeout() {
return timeout
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import hudson.model.Descriptor
import org.apache.commons.lang.StringUtils
import org.kohsuke.stapler.DataBoundConstructor

class PackageConfig extends AbstractDescribableImpl<PackageConfig> implements ExpandableConfig, Serializable {
class PackageConfig extends AbstractDescribableImpl<PackageConfig> implements ExpandableConfig, Serializable{

private static final long serialVersionUID = 1L

Expand All @@ -24,8 +24,12 @@ class PackageConfig extends AbstractDescribableImpl<PackageConfig> implements Ex
this.packageParameters = packageParameters ? removeEmptyParameters(packageParameters) : []
}

PackageConfig(PackageConfig config) {
this.packageParameters = config.getPackageParameters()
}

List<PackageParameter> getPackageParameters() {
return packageParameters
return packageParameters.collect({new PackageParameter(it)})
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ class TestConfig extends AbstractDescribableImpl<TestConfig> implements Expandab
this.constants = []
}

TestConfig(TestConfig config) {
this.tbcPath = config.getTbcPath()
this.tcfPath = config.getTcfPath()
this.constants = config.getConstants()
this.forceConfigurationReload = config.forceConfigurationReload
}

String getTbcPath() {
return tbcPath
}
Expand Down Expand Up @@ -62,7 +69,7 @@ class TestConfig extends AbstractDescribableImpl<TestConfig> implements Expandab
}

List<Constant> getConstants() {
return constants
return constants.collect({new Constant(it)})
}

@DataBoundSetter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ class AdditionalSetting extends AbstractDescribableImpl<AdditionalSetting> imple
this.value = StringUtils.trimToEmpty(value)
}

AdditionalSetting(AdditionalSetting setting) {
this.name = setting.getName()
this.value = setting.getValue()
}

String getName() {
return name
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ class Constant extends AbstractDescribableImpl<Constant> implements ExpandableCo
this.value = StringUtils.trimToEmpty(value)
}

Constant(Constant constant) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we use the props and instantiate a new object instead of adding a new constructor
new Constant(oldConst.getLabel(), olConst.getValue)
Same for AdditionalSettings and *Configs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We agreed on using copy constructors :)

this.label = constant.getLabel()
this.value = constant.getValue()
}

String getLabel() {
return label
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ class PackageParameter extends AbstractDescribableImpl<PackageParameter> impleme
this.value = StringUtils.trimToEmpty(value)
}

PackageParameter(PackageParameter param) {
this.label = param.getLabel()
this.value = param.getValue()
}

String getLabel() {
return label
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ class RecordingAsSetting extends AbstractDescribableImpl<RecordingAsSetting> imp
this.formatDetails = ''
}

RecordingAsSetting(RecordingAsSetting setting) {
this.path = setting.getPath()
this.recordingGroup = setting.getRecordingGroup()
this.mappingNames = setting.getMappingNames()
this.deviceName = setting.getDeviceName()
this.formatDetails = setting.getFormatDetails()
}

String getPath() {
return path
}
Expand All @@ -51,7 +59,7 @@ class RecordingAsSetting extends AbstractDescribableImpl<RecordingAsSetting> imp
}

List<String> getMappingNames() {
return mappingNames
return mappingNames.collect()
}

@DataBoundSetter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ class TestResult implements Serializable {
this.reportDir = reportDir
}

TestResult(TestResult result) {
this.reportId = result.getReportId()
this.reportDir = result.getReportDir()
this.testResult = result.getTestResult()
}

@Whitelisted
String getReportId() {
return reportId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ abstract class AbstractTestScanner {
* {@link hudson.remoting.Callable} providing remote access to scan a directory with a include file pattern.
*/
private static final class ScanTestCallable extends MasterToSlaveCallable<List<String>, IOException> {

private static final long serialVersionUID = 1L

private final String inputDir
private final filePattern

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class GenerateReportsStep extends Step {
}

List<AdditionalSetting> getAdditionalSettings() {
return additionalSettings
return additionalSettings.collect({new AdditionalSetting(it)})
}

@DataBoundSetter
Expand All @@ -54,7 +54,7 @@ class GenerateReportsStep extends Step {
}

List<String> getReportIds() {
return reportIds
return reportIds.collect()
}

@DataBoundSetter
Expand Down Expand Up @@ -85,6 +85,8 @@ class GenerateReportsStep extends Step {

static class Execution extends SynchronousNonBlockingStepExecution<GenerationResult> {

private static final long serialVersionUID = 1L

private final transient GenerateReportsStep step

Execution(GenerateReportsStep step, StepContext context) {
Expand All @@ -104,6 +106,8 @@ class GenerateReportsStep extends Step {
}

private static final class ExecutionCallable extends MasterToSlaveCallable<GenerationResult, IOException> {

private static final long serialVersionUID = 1L

private final String generatorName
private final Map<String, String> additionalSettings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class RunPackageStep extends RunTestStep {

@Nonnull
PackageConfig getPackageConfig() {
return packageConfig
return new PackageConfig(packageConfig)
}

@DataBoundSetter
Expand All @@ -61,7 +61,7 @@ class RunPackageStep extends RunTestStep {

@Nonnull
AnalysisConfig getAnalysisConfig() {
return analysisConfig
return new AnalysisConfig(analysisConfig)
}

@DataBoundSetter
Expand All @@ -75,6 +75,8 @@ class RunPackageStep extends RunTestStep {
}

static class Execution extends SynchronousNonBlockingStepExecution<TestResult> {

private static final long serialVersionUID = 1L

private final transient RunPackageStep step

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class RunProjectStep extends RunTestStep {

static class Execution extends SynchronousNonBlockingStepExecution<TestResult> {

private static final long serialVersionUID = 1L

private final transient RunProjectStep step

Execution(RunProjectStep step, StepContext context) {
Expand Down
Loading