-
-
Notifications
You must be signed in to change notification settings - Fork 517
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
Don't fail GitLab pipeline on QualityGate failure - optional property #637
Closed
szpak
wants to merge
4
commits into
mc1arke:master
from
szpak-forks:137-doNotFailGitLabPipelineSwitch
Closed
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
82828a5
[#137][GitLab] Extra property to disable pipeline failing on failed a…
szpak 087b5d9
[#137][GitLab] Simplify GitLab tests with junit-jupiter-params
szpak e23009c
[#137][GitLab] Tweaks after GitlabMergeRequestDecoratorTest migration…
szpak 5baff3e
[#137][GitLab] Adjust property name to be passed to Sonar
szpak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,11 @@ | |
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisIssueSummary; | ||
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisSummary; | ||
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.ReportGenerator; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.Arguments; | ||
import org.junit.jupiter.params.provider.MethodSource; | ||
import org.mockito.ArgumentCaptor; | ||
import org.sonar.api.ce.posttask.QualityGate; | ||
import org.sonar.api.issue.Issue; | ||
|
@@ -56,6 +59,7 @@ | |
import java.util.Collections; | ||
import java.util.Optional; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
@@ -69,7 +73,7 @@ | |
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.when; | ||
|
||
public class GitlabMergeRequestDecoratorTest { | ||
class GitlabMergeRequestDecoratorTest { | ||
|
||
private static final long MERGE_REQUEST_IID = 123; | ||
private static final long PROJECT_ID = 101; | ||
|
@@ -101,8 +105,8 @@ public class GitlabMergeRequestDecoratorTest { | |
|
||
private final GitlabMergeRequestDecorator underTest = new GitlabMergeRequestDecorator(scmInfoRepository, gitlabClientFactory, reportGenerator, markdownFormatterFactory); | ||
|
||
@Before | ||
public void setUp() throws IOException { | ||
@BeforeEach | ||
void setUp() throws IOException { | ||
when(analysisSummary.format(any())).thenReturn("Summary Comment"); | ||
when(reportGenerator.createAnalysisSummary(any())).thenReturn(analysisSummary); | ||
AnalysisIssueSummary analysisIssueSummary = mock(AnalysisIssueSummary.class); | ||
|
@@ -132,12 +136,12 @@ public void setUp() throws IOException { | |
} | ||
|
||
@Test | ||
public void shouldReturnCorrectDecoratorType() { | ||
void shouldReturnCorrectDecoratorType() { | ||
assertThat(underTest.alm()).containsOnly(ALM.GITLAB); | ||
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenPullRequestKeyNotNumeric() { | ||
void shouldThrowErrorWhenPullRequestKeyNotNumeric() { | ||
when(analysisDetails.getPullRequestId()).thenReturn("non-MR-IID"); | ||
|
||
assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
|
@@ -146,7 +150,7 @@ public void shouldThrowErrorWhenPullRequestKeyNotNumeric() { | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOException { | ||
void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOException { | ||
when(gitlabClient.getMergeRequest(any(), anyLong())).thenThrow(new IOException("dummy")); | ||
|
||
assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
|
@@ -155,7 +159,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOExce | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException { | ||
void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException { | ||
when(gitlabClient.getCurrentUser()).thenThrow(new IOException("dummy")); | ||
|
||
assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
|
@@ -164,7 +168,7 @@ public void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException { | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws IOException { | ||
void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws IOException { | ||
when(gitlabClient.getMergeRequestCommits(anyLong(), anyLong())).thenThrow(new IOException("dummy")); | ||
|
||
assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
|
@@ -173,7 +177,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() throws IOException { | ||
void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() throws IOException { | ||
when(gitlabClient.getMergeRequestDiscussions(anyLong(), anyLong())).thenThrow(new IOException("dummy")); | ||
|
||
assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
|
@@ -182,7 +186,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() thr | |
} | ||
|
||
@Test | ||
public void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException { | ||
void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("Post with no issue ID"); | ||
|
@@ -203,7 +207,7 @@ public void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoI | |
assertThat(mergeRequestNoteArgumentCaptor.getValue()).isNotInstanceOf(CommitNote.class); } | ||
|
||
@Test | ||
public void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException { | ||
void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("Post with no issue ID"); | ||
|
@@ -221,7 +225,7 @@ public void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUser | |
} | ||
|
||
@Test | ||
public void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUserButNoId() throws IOException { | ||
void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUserButNoId() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("Another post with no issue ID\nbut containing a new line"); | ||
|
@@ -249,7 +253,7 @@ public void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUser | |
} | ||
|
||
@Test | ||
public void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySystemNoteFromOtherUser() throws IOException { | ||
void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySystemNoteFromOtherUser() throws IOException { | ||
User otherUser = mock(User.class); | ||
when(otherUser.getUsername()).thenReturn("[email protected]"); | ||
|
||
|
@@ -279,7 +283,7 @@ public void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySyste | |
} | ||
|
||
@Test | ||
public void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithNoId() throws IOException { | ||
void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithNoId() throws IOException { | ||
User otherUser = mock(User.class); | ||
when(otherUser.getUsername()).thenReturn("[email protected]"); | ||
|
||
|
@@ -310,7 +314,7 @@ public void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSona | |
} | ||
|
||
@Test | ||
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithNoId() throws IOException { | ||
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithNoId() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("And another post with no issue ID\nNo View in SonarQube link"); | ||
|
@@ -339,7 +343,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote | |
} | ||
|
||
@Test | ||
public void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithIssuedId() throws IOException { | ||
void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithIssuedId() throws IOException { | ||
User otherUser = mock(User.class); | ||
when(otherUser.getUsername()).thenReturn("[email protected]"); | ||
|
||
|
@@ -371,7 +375,7 @@ public void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSo | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOException { | ||
void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOException { | ||
User otherUser = mock(User.class); | ||
when(otherUser.getUsername()).thenReturn("[email protected]"); | ||
|
||
|
@@ -406,7 +410,7 @@ public void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOExcep | |
} | ||
|
||
@Test | ||
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithIssueId() throws IOException { | ||
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithIssueId() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("And another post with an issue ID\n[View in SonarQube](url)"); | ||
|
@@ -435,7 +439,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote | |
} | ||
|
||
@Test | ||
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeWithOtherProjectId() throws IOException { | ||
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeWithOtherProjectId() throws IOException { | ||
Note note = mock(Note.class); | ||
when(note.getAuthor()).thenReturn(sonarqubeUser); | ||
when(note.getBody()).thenReturn("And another post with an issue ID\n[View in SonarQube](url)"); | ||
|
@@ -464,7 +468,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOException { | ||
void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOException { | ||
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class); | ||
when(lightIssue.key()).thenReturn("issueKey1"); | ||
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN); | ||
|
@@ -506,7 +510,7 @@ public void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOExcepti | |
} | ||
|
||
@Test | ||
public void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws IOException { | ||
void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws IOException { | ||
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class); | ||
when(lightIssue.key()).thenReturn("issueKey1"); | ||
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN); | ||
|
@@ -545,7 +549,7 @@ public void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws | |
} | ||
|
||
@Test | ||
public void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMergeRequest() throws IOException { | ||
void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMergeRequest() throws IOException { | ||
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class); | ||
when(lightIssue.key()).thenReturn("issueKey1"); | ||
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN); | ||
|
@@ -590,7 +594,7 @@ public void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMe | |
} | ||
|
||
@Test | ||
public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOException { | ||
void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOException { | ||
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class); | ||
when(lightIssue.key()).thenReturn("issueKey1"); | ||
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN); | ||
|
@@ -618,7 +622,7 @@ public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOExcepti | |
} | ||
|
||
@Test | ||
public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSuccessAnalysis() throws IOException { | ||
void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSuccessAnalysis() throws IOException { | ||
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.OK); | ||
when(analysisDetails.getCommitSha()).thenReturn("commitsha"); | ||
|
||
|
@@ -646,11 +650,15 @@ public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSucce | |
PipelineStatus.State.SUCCESS, "https://sonarqube.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, null, null)); | ||
} | ||
|
||
@Test | ||
public void shouldSubmitFailedPipelineStatusAndUnresolvedSummaryCommentOnFailedAnalysis() throws IOException { | ||
@SuppressWarnings({"OptionalUsedAsFieldOrParameterType", "unused"}) | ||
@MethodSource | ||
@ParameterizedTest //https://github.com/mc1arke/sonarqube-community-branch-plugin/issues/137 | ||
void shouldSubmitRequestedPipelineStatusBasedOnPropertiesAndUnresolvedSummaryCommentOnFailedAnalysis( | ||
Optional<String> dontFailPipelinePropertyValue, PipelineStatus.State expectedSentPipelineStatus, String description) throws IOException { | ||
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR); | ||
when(analysisDetails.getCommitSha()).thenReturn("other sha"); | ||
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11")); | ||
when(analysisDetails.getScannerProperty("sonar.analysis.com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.dontFailPipeline")).thenReturn(dontFailPipelinePropertyValue); | ||
|
||
when(analysisSummary.format(any())).thenReturn("Different Summary comment"); | ||
when(analysisSummary.getDashboardUrl()).thenReturn("https://sonarqube2.dummy/dashboard?id=projectKey&pullRequest=123"); | ||
|
@@ -674,11 +682,20 @@ public void shouldSubmitFailedPipelineStatusAndUnresolvedSummaryCommentOnFailedA | |
assertThat(pipelineStatusArgumentCaptor.getValue()) | ||
.usingRecursiveComparison() | ||
.isEqualTo(new PipelineStatus("SonarQube", "SonarQube Status", | ||
PipelineStatus.State.FAILED, "https://sonarqube2.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, BigDecimal.TEN, 11L)); | ||
expectedSentPipelineStatus, "https://sonarqube2.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, BigDecimal.TEN, 11L)); | ||
} | ||
|
||
@SuppressWarnings("unused") //used by @ParameterizedTest | ||
private static Stream<Arguments> shouldSubmitRequestedPipelineStatusBasedOnPropertiesAndUnresolvedSummaryCommentOnFailedAnalysis() { | ||
return Stream.of( | ||
Arguments.of(Optional.empty(), PipelineStatus.State.FAILED, "dontFailPipeline not defined"), | ||
Arguments.of(Optional.of("true"), PipelineStatus.State.SUCCESS, "dontFailPipeline == true"), | ||
Arguments.of(Optional.of("false"), PipelineStatus.State.FAILED, "dontFailPipeline == false") | ||
); | ||
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOException { | ||
void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOException { | ||
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR); | ||
when(analysisDetails.getCommitSha()).thenReturn("other sha"); | ||
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11")); | ||
|
@@ -712,7 +729,7 @@ public void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOExc | |
} | ||
|
||
@Test | ||
public void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException { | ||
void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException { | ||
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR); | ||
when(analysisDetails.getCommitSha()).thenReturn("other sha"); | ||
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11")); | ||
|
@@ -739,14 +756,14 @@ public void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException | |
} | ||
|
||
@Test | ||
public void shouldReturnWebUrlFromMergeRequestIfScannerPropertyNotSet() { | ||
void shouldReturnWebUrlFromMergeRequestIfScannerPropertyNotSet() { | ||
assertThat(underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
.usingRecursiveComparison() | ||
.isEqualTo(DecorationResult.builder().withPullRequestUrl(MERGE_REQUEST_WEB_URL).build()); | ||
} | ||
|
||
@Test | ||
public void shouldReturnWebUrlFromScannerPropertyIfSet() { | ||
void shouldReturnWebUrlFromScannerPropertyIfSet() { | ||
when(analysisDetails.getScannerProperty("sonar.pullrequest.gitlab.projectUrl")).thenReturn(Optional.of(MERGE_REQUEST_WEB_URL + "/additional")); | ||
assertThat(underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto)) | ||
.usingRecursiveComparison() | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a facility we don't really want: someone who has access to submit scans can decide whether the pipeline passes on a Quality Gate failure, rather than it being clear that a passing pipeline is because of clean code. In the companies I work with, the decision is made up-front as to what rules must be adhered to, and it is assumed that a pipeline is passing because those rules have been met. If we start allowing people who control the scanner to decide whether they can bypass Quality Checks then we'd effectively be breaking the safeguard that Sonarqube is providing in these scenarios.