-
Notifications
You must be signed in to change notification settings - Fork 57
Add possibility to create Default Reviewers #82
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
Conversation
…o defaultReviewers
…o defaultReviewers-list
# Conflicts: # src/test/java/com/cdancy/bitbucket/rest/features/DefaultReviewersApiLiveTest.java
…o defaultReviewers-list
…o defaultReviewers-list # Conflicts: # src/test/java/com/cdancy/bitbucket/rest/features/RepositoryApiLiveTest.java
# Conflicts: # src/test/java/com/cdancy/bitbucket/rest/features/BranchApiLiveTest.java # src/test/java/com/cdancy/bitbucket/rest/features/DefaultReviewersApiLiveTest.java
Matcher matcherSrc = Matcher.create(Matcher.MatcherId.ANY, true); | ||
Matcher matcherDst = Matcher.create(Matcher.MatcherId.ANY, true); | ||
List<User> listUser = new ArrayList<>(); | ||
listUser.add(User.create("test", "test@test.com", 1, "test", true, "test", "NORMAL")); |
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.
Will this work if you just add any user? If not we can always use the default logged in user we are running these tests with.
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.
Ok, I will change it for defaultUser.
|
||
private GeneratedTestContents generatedTestContents; | ||
|
||
|
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.
Remove new line?
@Test(groups = "live", testName = "DefaultReviewersApiLiveTest", singleThreaded = true) | ||
public class DefaultReviewersApiLiveTest extends BaseBitbucketApiLiveTest { | ||
|
||
|
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.
Remove extra newline?
…o defaultReviewers-create # Conflicts: # src/main/java/com/cdancy/bitbucket/rest/domain/defaultreviewers/Condition.java # src/main/java/com/cdancy/bitbucket/rest/features/DefaultReviewersApi.java # src/test/java/com/cdancy/bitbucket/rest/features/DefaultReviewersApiLiveTest.java # src/test/java/com/cdancy/bitbucket/rest/features/DefaultReviewersApiMockTest.java
return defaultUserAsString; | ||
} | ||
|
||
protected synchronized User getDefaultUser() { |
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 like this better. Would you mind just removing the defaultUserAsString
and we will standardize on just using the User
object across the board. All classes can then just query the User
object for its actual name.
if (username.equals(user.slug())) { | ||
defaultUser = user; | ||
break; | ||
} | ||
} |
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 would just return once we've found the correct user and throw an exception if somehow we get past the for loop.
validCondition(returnCondition, requiredApprover, Matcher.MatcherId.ANY_REF, Matcher.MatcherId.ANY_REF); | ||
} | ||
|
||
@Test(dependsOnMethods = {"testListDefaultReviewersOnNewRepo", "testCreateCondition"}) |
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.
We only need to depend on method testCreateCondition
as it in turn depends on method testListDefaultReviewersOnNewRepo
.
validCondition(returnCondition, requiredApprover, Matcher.MatcherId.MASTER, Matcher.MatcherId.DEVELOPMENT); | ||
} | ||
|
||
@Test(dependsOnMethods = {"testListDefaultReviewersOnNewRepo", "testCreateCondition", "testCreateConditionMatcherDifferent"}) |
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.
We only need to depend on method testCreateConditionMatcherDifferent
if I'm not mistaken?
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.
No, I need to depend on method testCreateCondition and testCreateConditionMatcherDifferent because testListConditions do check if It have 2 conditions return by api. I will remove third.
LGTM. @j0nathan33 merging this now but am making one minor change to the |
No description provided.