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

simplified some assertions by using #containsExactly #68

Merged
merged 1 commit into from
Feb 18, 2016

Conversation

PascalSchumacher
Copy link
Contributor

I think it is more readable and it gives a nicer failure message, but feel free to reject if you disagree. :)

@aaschmid
Copy link
Member

Hi @PascalSchumacher, thanks for the effort and changes. For same parts I really like to explicitly use the array and equals notation to make the test case clearer but at some you are perfectly correct to use "containsExactly" instead. Is there a way for me to change the pull request before merging? On the other hand I will thing about it before getting back to my laptop ;-)

@PascalSchumacher
Copy link
Contributor Author

Thanks for the quick feedback. :)

You can change everything about a (pull request) commit when you cherry-pick and then amend. It will still show me as the author and you as the committer of the commit.

e.g.:

git fetch origin pull/68/head:simplify_some_assertions
git cherry-pick 62fb1fe3169184a8538910a5b13e1fccbda145e5
do changes you want
git commit --amend

If you add closes #68 to the commit comment github will close the pull request when you push.

You can also just comment on the changes you want me to undo and I will update the pull request myself.

@aaschmid
Copy link
Member

No, thanks for the pull request. I will have a closer look on the weekend as this is not urgent :-)

@PascalSchumacher
Copy link
Contributor Author

Yes, it's not urgent at all. So take you time :-)

@@ -323,8 +315,7 @@ public void testConvertShouldReturnOneElementForStringArrayWithOneElementSplitBy
List<Object[]> result = underTest.convert(data, false, parameterTypes, dataProvider);

// Then:
assertThat(result).hasSize(1);
assertThat(result.get(0)).isEqualTo(new Object[] { "foo", true });
assertThat(result).containsExactly(new Object[] { "foo", true });
Copy link
Member

Choose a reason for hiding this comment

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

maybe we cloud use a helper for creating the array instead of using new Object[] { ... } over and over.
Do you also think that it would be better for the test cases above to write down the data explicitly instead of reusing the test data somehow?

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'm a fan of creating helper methods for tests. So if it's possible to come up with a descriptive method name I'm for adding it. Maybe something like assertContainsOnlyObjectArrayWithValues(List, values...)?

I agree that it would be better to write down the data explicitly in the test above.

aaschmid added a commit that referenced this pull request Feb 18, 2016
simplified some assertions by using #containsExactly
@aaschmid aaschmid merged commit 86aba49 into TNG:master Feb 18, 2016
@aaschmid
Copy link
Member

After all thanks for the PR and the discussions :-)
I just merged it even if I don't like all the places. I am more considering a new version based on the new junit-lamba rather than investing too much time in this quite stable one ;-)

@aaschmid aaschmid added this to the v1.10.3 milestone Feb 18, 2016
@aaschmid aaschmid self-assigned this Feb 18, 2016
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.

None yet

2 participants