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

James 2121Disallow creation of several identical mapping #953

Closed
wants to merge 7 commits into from

Conversation

rouazana
Copy link

And some cosmetic changes

@@ -328,6 +328,25 @@ public void sortMappingsShouldPutDomainAliasFirstWhenVariousMappings() {
.build());
}

@Test
public void addMappingShouldThrowWhenMappingAlreadyExists() throws Exception {
Copy link

Choose a reason for hiding this comment

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

shouldThrow? but it has assertThat?

Copy link
Author

Choose a reason for hiding this comment

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

yes, some tests methods are swallowing exceptions and transform them to boolean (false in case of exception or failure). I made the changes to have proper test changes, but they fail with hbase because of the incompatibility of Guave version (cannot found MoreObject). I don't want to go deeper into this, so I let it like that.

Copy link
Author

Choose a reason for hiding this comment

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

finally the fix was needed, so fixed

String address = "test@localhost2";

assertThat(addMapping(user, domain, address, ADDRESS_TYPE)).isTrue();
assertThat(addMapping(user, domain, address, ADDRESS_TYPE)).isFalse();

Choose a reason for hiding this comment

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

what line is the when and what line it then ?

Copy link
Author

Choose a reason for hiding this comment

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

first is when and second is then? :)
no seriously intent would be clearer with exception but see my previous comment

@rouazana
Copy link
Author

sorry I had to rebase to fix some conflicts with guava optionals removing

@rouazana
Copy link
Author

rouazana commented Sep 1, 2017

merged

@rouazana rouazana closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants