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

Add possibility to get branch Model Condition #68

Merged
merged 6 commits into from
May 25, 2017

Conversation

j0nathan33
Copy link
Contributor

No description provided.

@cdancy cdancy added this to the v0.0.16 milestone May 25, 2017
@@ -0,0 +1,50 @@
/*
Copy link
Owner

Choose a reason for hiding this comment

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

Should this file be here? Doesn't seem to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use in another PR (#69).

@AfterClass
public void fin() {
boolean success = api().updateDefault(projectKey, repoKey, defaultBranchId);
assertThat(success).isTrue();
success = api().delete(projectKey, repoKey, "refs/heads/" + branchName);
assertThat(success).isTrue();
if (branchModelConfiguration != null) {
checkDefaultBranchConfiguration();
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need to do this check twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, This line is for update

@Test(dependsOnMethods = {"testCreateBranch", "testListBranches"})
public void testGetBranchModelConfiguration() {
branchModelConfiguration = api().getModelConfiguration(projectKey, repoKey);
checkDefaultBranchConfiguration();
Copy link
Owner

Choose a reason for hiding this comment

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

If we don't need to do this check twice than lets remove the checkDefaultBranchConfiguration method and instead add that code here.

Copy link
Owner

Choose a reason for hiding this comment

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

Seen you pushed a commit but missed this. Not sure if you were planning on addressing with a subsequent commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove it, I need to readd it in another PR (#69)

Copy link
Owner

Choose a reason for hiding this comment

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

Gotcha ... yeah looking at that PR now. Ok lets keep it then.

@@ -17,17 +17,16 @@

Copy link
Owner

Choose a reason for hiding this comment

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

Another LiveTest to check for errors? Maybe named something like testGetBranchModelConfigurationOnError()?

private void checkDefaultBranchConfiguration() {
assertThat(branchModelConfiguration).isNotNull();
assertThat(branchModelConfiguration.errors().isEmpty()).isTrue();
assertThat(branchModelConfiguration.development().refId()).isNotNull();
Copy link
Owner

Choose a reason for hiding this comment

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

This was actually null when I ran the tests. Seeing as how annotated with @nullable not sure if we should check for it here? I'm going to merge this now and remove the check. If you have a strong objection to this let me know and we can follow up on #69

@cdancy cdancy merged commit f9ffc63 into cdancy:master May 25, 2017
@j0nathan33 j0nathan33 deleted the branchModel-list branch May 26, 2017 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants