Add Failing Build Probe #422
Conversation
AayushSaini101
force-pushed
the
Issue-363
branch
from
3042cce
to
d59ed6b
Compare
| @Test | ||
| void shouldCorrectlyDetectMissingJenkinsfile() throws IOException { | ||
| final FailingBuildProbe probe = getSpy(); | ||
| final Plugin plugin = mock(Plugin.class); | ||
| final ProbeContext ctx = mock(ProbeContext.class); | ||
| final Path repo = Files.createTempDirectory("foo"); | ||
| when(ctx.getScmRepository()).thenReturn(Optional.of(repo)); | ||
| assertThat(probe.apply(plugin, ctx)).withFailMessage("No JenkinsFile found"); | ||
| } |
There was a problem hiding this comment.
I think this is already taking care off by the Jenkinsfile probe.
What I think would be beneficial for this pull request is to validate the behaviour when we cannot get the run of the latest commit of the default branch, to make sure which "run" you are getting with the iterator().next(), if it's possible for the conclusion to be null.
There was a problem hiding this comment.
Thanks @alecharp Is the above logic to check the status of the default branch in the probe is not correct ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, @alecharp
| @Order(FailingBuildProbe.ORDER) | ||
| public class FailingBuildProbe extends Probe { | ||
|
|
||
| public static final int ORDER = LastCommitDateProbe.ORDER + 100; |
There was a problem hiding this comment.
Run it after the Jenkinsfile probe and you can use its result to make sure there is a Jenkinsfile present.
There was a problem hiding this comment.
Done @alecharp
| public ProbeResult repoContainsJenkins(ProbeContext context) { | ||
| final Path repository = context.getScmRepository().get(); | ||
| try (Stream<Path> paths = Files.find(repository, 1, (file, $) -> | ||
| Files.isReadable(file) && "JenkinsFile".equals(file.getFileName().toString()))) { | ||
| return paths.findFirst() | ||
| .map(file -> this.success("Jenkinsfile found")) | ||
| .orElseGet(() -> this.error("No Jenkinsfile found")); | ||
| } catch (IOException e) { | ||
| return this.error(e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is doing the Jenkinsfile probe job again. Not sure this is necessary.
There was a problem hiding this comment.
Done @alecharp Removed and update the order
| GHCommit commit = repository.getCommit(defaultBranch); | ||
| GHCheckRun checkRun = commit.getCheckRuns().iterator().next(); | ||
| GHCheckRun.Conclusion conclusion = checkRun.getConclusion(); | ||
|
|
||
| if (conclusion == GHCheckRun.Conclusion.FAILURE) { | ||
| return this.success("Build Failed in Default Branch"); | ||
| } else { | ||
| return this.success("Build is Success in Default Branch"); | ||
| } |
There was a problem hiding this comment.
Those statements need to be tested.
There was a problem hiding this comment.
@alecharp can you help me in the testing, GHCommit ghCommit = new GHCommit(); i cannot set particular commits for testing. any kind of more generic way to test it
core/src/main/java/io/jenkins/pluginhealth/scoring/probes/FailingBuildProbe.java
Outdated
Show resolved
Hide resolved
|
@AayushSaini101 Are you still interesting to drive this pull request? |
Yes, @alecharp I can work on this. Will submit the changes by ASAP |
|
Closing this @alecharp due to multiple accounts. Creating new one |
|
Please, stop doing that. Use the proper account from the start. You are wasting the review the discussion already started here. |
Sorry, This was my last PR that contained issue due to multiple accounts, I have resolved the issues in the remaining ones. |
|
@AayushSaini101 have you plan to attend this pull request? |
Sorry @alecharp for being late for this PR, I am working on the GSOC proposal for this project, I will try to complete by this week. |
|
@alecharp Can you help me to write test case for the changes, I am not sure how can i test the changes properly |
There was a problem hiding this comment.
Missing the license as well
AayushSaini101
force-pushed
the
Issue-363
branch
from
cf16823
to
7f88c4f
Compare
|
Please, do not force push branch on pull request that has already been reviewed. There is still no tests to validate the probe. |
AayushSaini101
force-pushed
the
Issue-363
branch
from
7f88c4f
to
820233f
Compare
@alecharp I tried to push normally but cannot able to push even rebase and resolving the conflicts, there were multiple accounts in the last commits, may be the reason of this, I have tried many approaches to push normally but cannot, Sorry for this |
|
After removing the multiple accounts commits, now i can able to push normally |
| Conclusion conclusion = checkRun.getConclusion(); | ||
|
|
||
| if (conclusion == Conclusion.FAILURE) { | ||
| return this.success("Build Failed in Default Branch"); |
There was a problem hiding this comment.
| return this.success("Build Failed in Default Branch"); | |
| return this.success("Build failed in default branch"); |
| if (conclusion == Conclusion.FAILURE) { | ||
| return this.success("Build Failed in Default Branch"); | ||
| } else { | ||
| return this.success("Build is Success in Default Branch"); |
There was a problem hiding this comment.
| return this.success("Build is Success in Default Branch"); | |
| return this.success("Build is successful in default branch"); |
|
|
||
| final Optional<String> repositoryName = context.getRepositoryName(); | ||
| final GHRepository ghRepository = context.getGitHub().getRepository(repositoryName.get()); | ||
| GHCommit commit = ghRepository.getCommit(ucPlugin.defaultBranch()); |
There was a problem hiding this comment.
Value already assigned to a variable, let's use it.
| GHCommit commit = ghRepository.getCommit(ucPlugin.defaultBranch()); | |
| GHCommit commit = ghRepository.getCommit(defaultBranch); |
| } else { | ||
| return this.success("Build is Success in Default Branch"); | ||
| } | ||
| } catch (Exception ex) { |
There was a problem hiding this comment.
If we catch Exception, it means to catch anything that could go wrong. And we wouldn't know what went wrong.
Please, be more specific than Exception and log the exception.
| @Order(DefaultBranchBuildStatusProbe.ORDER) | ||
| public class DefaultBranchBuildStatusProbe extends Probe { | ||
|
|
||
| public static final int ORDER = JenkinsCoreProbe.ORDER + 100; |
There was a problem hiding this comment.
Do we need to be executed after that probe?
| @BeforeEach | ||
| public void init() { | ||
|
|
||
| probe = getSpy(); | ||
| } |
There was a problem hiding this comment.
Why do we need to have the probe in a field?
| final Plugin plugin = mock(Plugin.class); | ||
| final ProbeContext ctx = mock(ProbeContext.class); | ||
|
|
||
| final GitHub gh = mock(GitHub.class); |
There was a problem hiding this comment.
Already mocked on line 74.
| final GitHub gh = mock(GitHub.class); |
| List.of())); | ||
| when(ctx.getGitHub()).thenReturn(gh); | ||
| when(ctx.getRepositoryName()).thenReturn(Optional.of(pluginRepo)); | ||
| when(ctx.getGitHub()).thenReturn(gitHub); |
There was a problem hiding this comment.
@alecharp I didn't get you regarding this
There was a problem hiding this comment.
Yes, I meant useless. you already mock that class before.
Description
Closes #363
Add a new probe to check whether the build failed on the the default branch or not.
Testing done
Added new test cases for testing the probe.
Submitter checklist