-
Notifications
You must be signed in to change notification settings - Fork 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.
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
add func test for running actions from bitbucket server #220
Conversation
…ver-integration-plugin into BBSDEV-22979-run-actions-func-tests
…unning builds from bitbucket
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.
I think this looks good overall, there are some minor things I think we can improve on as detailed in my comments.
@@ -60,6 +64,9 @@ public void cancel() { | |||
|
|||
private void openAndVerify() { | |||
open(); | |||
if (this.requestToken == null) { |
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.
is this safe? As in, does this not reduce the quality of the test? Will it not mean that if the requestToken
is null it will pass the test? I don't see we're handling the null token elsewhere?
String jobBuildPostUrl = String.format("%s/job/%s/build", removeEnd(getBaseUrl(), "/"), job.name); | ||
OAuthRequest postBuildStatusOAuthRequest = new OAuthRequest(Verb.POST, jobBuildPostUrl); | ||
postBuildStatusOAuthRequest.addHeader("Accept", "application/json"); | ||
oAuthClient.execute(postBuildStatusOAuthRequest, userAccessToken); |
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.
maybe we can use the Jenkins REST "api" to trigger the build, rather than using an Oauth client. I find this is a bit confusing and I struggle to get my head around what is going on.
)); | ||
|
||
// Applink Bitbucket Server and Jenkins over REST | ||
applinkUrl = createStashApplicationLink("generic", "Jenkins testing", jenkins.url.toString(), jenkins.url.toString()); |
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.
Maybe consider extending the createStashApplicationLink()
and put all the set-up stuff in there. It makes this test easier to read as there is less code here.
@@ -44,6 +44,12 @@ | |||
<groupId>org.jenkins-ci</groupId> | |||
<artifactId>acceptance-test-harness</artifactId> | |||
<version>${jenkins.acceptance-test-harness.version}</version> | |||
<exclusions> | |||
<exclusion> | |||
<groupId>org.slf4j</groupId> |
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.
I think we should add in an explicit dependency on slf4j as well.
@@ -27,6 +27,10 @@ | |||
|
|||
private final String requestToken; | |||
|
|||
public OAuthAuthorizeTokenPage(Jenkins context, URL pageUrl) { | |||
this(context, pageUrl, null); |
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.
as we spoke about. See if you can with relative ease get the requestToken
from the URL
to ensure that all the tests continue to provide maximum value.
|
||
@WithPlugins({"mailer", "atlassian-bitbucket-server-integration"}) | ||
@WithPlugins({"atlassian-bitbucket-server-integration", "mailer", "matrix-auth"}) | ||
public class SmokeTest extends AbstractJUnitTest { |
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.
As a general thought it feels like this test is now a bit big. there are over 500 lines of code here. Maybe we can move some of the tests to a different file to group them more? It is a minor point.
} | ||
|
||
@Test | ||
public void testRunBuildActionWtihFreestlyeJob() throws Exception { |
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 test is huge, as we discussed I think it would benefit a lot from moving things out to methods that are not immediately test related. Things that come to mind are setting up applink, configuring the environment, authorise the server. You can then call those methods from here and the test is slimmer and easier to read.
@@ -2,11 +2,16 @@ | |||
|
|||
import com.github.scribejava.core.model.*; | |||
import com.google.common.collect.ImmutableMap; | |||
import io.restassured.RestAssured; |
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.
all these added imports are unused. Please remove
.expect() | ||
.statusCode(200) | ||
.when() | ||
.post(getInternalCommitUrl(forkRepo, commitId) + "/build-operations"); |
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.
I noticed "running actions from bitbucket server" in the title of this pull request and thought that perhaps Bitbucket Server now has a feature similar to actions
in the Checks API of GitHub and ChecksAction
in the Checks API plugin for Jenkins, so that it could be used in jenkinsci/checks-api-plugin#65. Alas, this test apparently uses a REST resource that is intended for the JavaScript UI of Bitbucket Server itself and cannot define any new build operations.
No description provided.