Skip to content

Conversation

Tjuri
Copy link
Contributor

@Tjuri Tjuri commented Oct 27, 2021

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

AddedVersusDeletedLinesTrendChart builder = new AddedVersusDeletedLinesTrendChart();

Iterable<BuildResult<CommitStatisticsBuildAction>> buildResultsStub = createBuildResultsStub();
ChartModelConfiguration chartModelConfigurationStub = createConfiguration();
Copy link
Member

Choose a reason for hiding this comment

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

Use simple dummy object if possible:

Suggested change
ChartModelConfiguration chartModelConfigurationStub = createConfiguration();
ChartModelConfiguration chartModelConfigurationStub = new ChartModelConfiguration();

Comment on lines 50 to 57
private Iterable<BuildResult<CommitStatisticsBuildAction>> createBuildResultsStub() {
return new Iterable<BuildResult<CommitStatisticsBuildAction>>() {
@Override
public Iterator<BuildResult<CommitStatisticsBuildAction>> iterator() {
return null;
}
};
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be easier to replace with something like (see https://github.com/jenkinsci/forensics-api-plugin/blob/master/src/test/java/io/jenkins/plugins/forensics/miner/FilesCountTrendChartTest.java for a reference):

    private BuildResult<CommitStatisticsBuildAction> createResult(final int buildNumber, final int added, final int deleted) {
        CommitStatisticsBuildAction action = mock(CommitStatisticsBuildAction.class);
        CommitStatistics commitStatistics = mock(CommitStatistics.class);
        when(commitStatistics.getAddedLines()).thenReturn(added);
        when(commitStatistics.getDeletedLines()).thenReturn(deleted);

        when(action.getCommitStatistics()).thenReturn(commitStatistics);

        Build build = new Build(buildNumber);

        return new BuildResult<>(build, action);
    }

void shouldCreate() {
AddedVersusDeletedLinesTrendChart builder = new AddedVersusDeletedLinesTrendChart();

Iterable<BuildResult<CommitStatisticsBuildAction>> buildResultsStub = createBuildResultsStub();
Copy link
Member

Choose a reason for hiding this comment

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

This can be replaced by a simple list:

        List<BuildResult<CommitStatisticsBuildAction>> results = new ArrayList<>();
        results.add(createResult(2, 20, 10));
        results.add(createResult(1, 30, 25));

Comment on lines 25 to 29
SeriesBuilder seriesBuilderStub = createSeriesBuilderStub();
LinesDataSet linesDataSet = mock(LinesDataSet.class);

when(seriesBuilderStub.createDataSet(chartModelConfigurationStub, buildResultsStub))
.thenReturn(linesDataSet);
Copy link
Member

Choose a reason for hiding this comment

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

Not required anymore

Comment on lines 34 to 37
assertThat(lineChartModel)
.satisfies(model -> {
assertThat(model.getSeries()).size().isEqualTo(2);
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assertThat(lineChartModel)
.satisfies(model -> {
assertThat(model.getSeries()).size().isEqualTo(2);
});
assertThat(lineChartModel.getSeries()).hasSize(2);

.satisfies(model -> {
assertThat(model.getSeries()).size().isEqualTo(2);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

.satisfies(model -> {
assertThat(model.getSeries()).size().isEqualTo(2);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #351 (93e947e) into master (237fba3) will increase coverage by 5.67%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #351      +/-   ##
============================================
+ Coverage     50.04%   55.71%   +5.67%     
- Complexity      253      271      +18     
============================================
  Files            42       42              
  Lines          1093     1093              
  Branches         92       92              
============================================
+ Hits            547      609      +62     
+ Misses          510      447      -63     
- Partials         36       37       +1     
Impacted Files Coverage Δ
.../jenkins/plugins/forensics/miner/SizePieChart.java 86.66% <0.00%> (+86.66%) ⬆️
...s/plugins/forensics/miner/ForensicsTableModel.java 100.00% <0.00%> (+100.00%) ⬆️
...nsics/miner/AddedVersusDeletedLinesTrendChart.java 100.00% <0.00%> (+100.00%) ⬆️
...AddedVersusDeletedLinesForensicsSeriesBuilder.java 100.00% <0.00%> (+100.00%) ⬆️
...rsusDeletedLinesCommitStatisticsSeriesBuilder.java 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 237fba3...93e947e. Read the comment docs.

@uhafner uhafner changed the title Test for AddedVersusDeletedLinesTrendChart Add unit test for AddedVersusDeletedLinesTrendChart Nov 2, 2021
@uhafner uhafner added the tests Enhancement of tests label Nov 2, 2021
@Tjuri Tjuri requested a review from uhafner November 3, 2021 15:17
@uhafner uhafner merged commit cc504c4 into jenkinsci:master Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Enhancement of tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants