Skip to content

Conversation

@olcbean
Copy link
Contributor

@olcbean olcbean commented Jul 21, 2017

Replacing general assertThat by more concrete JUnit methods (assertEquals, assertTrue, etc).

Based on a review comment

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jasontedor
Copy link
Member

Sorry, I strongly prefer assertThat (it gives better error messages, and it's easier to not mixup expected and actual (which you've done in all the ones that I checked here)). I think we should not make this change, and in general while we appreciate and welcome all proposed changes, we have bigger priorities than changes like this. Thanks for the PR, but I'm going to close this one.

@jasontedor jasontedor closed this Jul 21, 2017
@javanna
Copy link
Member

javanna commented Jul 21, 2017

Sorry @olcbean my bad as I made the original comment and made you adapt your code in a PR. I thought that we were sold as a team in using assertEquals over assertThat. Funnily enough I was not in agreement with this outcome of some previous discussion, for the reasons that @jasontedor states above, but I just adhered to it at some point. Sorry about the confusion!

@olcbean olcbean deleted the remove_assertThat branch July 21, 2017 13:33
@olcbean
Copy link
Contributor Author

olcbean commented Jul 21, 2017

@jasontedor thanks for the clarification.
@javanna no worries. Now we are all on the same page ;)

In general I do not mind either. However in the codebase and the PRs both appear, hence this PR. But I will stick to assertThat in the future.

However I believe that sometimes assertThat is taken to extremes : asserThat(..., equalTo(true) is a little bit too much, isn't it?

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.

4 participants