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

Asserts for CharSequence contents #1432

Closed
sabi0 opened this issue Mar 18, 2017 · 15 comments
Closed

Asserts for CharSequence contents #1432

sabi0 opened this issue Mar 18, 2017 · 15 comments

Comments

@sabi0
Copy link

sabi0 commented Mar 18, 2017

A lot of APIs in Android deal with CharSequence. And using assertEquals() with CharSequence arguments is cumbersome and unstable. One has to reproduce the exact same type which might change later.
And sometimes it just does not work (e.g. StringBuilder uses default Object.equals() implementation).

Therefore I would suggest to add new methods like
assertContentEquals(CharSequence expected, CharSequence actual)
that would compare just the content, irregardless of the type it's been wrapped into.

(Pull request #949 was trying to do that with generic assertEquals() which is indeed questionable)

I can create a pull request if you find this proposal reasonable.

@kcooney
Copy link
Member

kcooney commented Mar 18, 2017

Important issue, but I would prefer to do it in assertEquals or an overload of assertEquals. Having to know which assert message to use for which use case is error-prone.

You might want to consider using assertion frameworks with a fluent API (like Truth or Fest).

@sabi0
Copy link
Author

sabi0 commented Mar 18, 2017

The problem with assertEquals is that sometimes you do need to compare the types too.
So I believe assertEquals("abc", new StringBuilder("abc")) must fail. Therefore the need for separate methods to compare just the content.
This is IMO very similar to the current assertArrayEquals approach.

I agree that picking the right method would become a bit trickier. But it's anyway better than the current lack of proper CharSequence support.

@kcooney
Copy link
Member

kcooney commented Mar 18, 2017

Could you list what specific methods you are proposing?

@sabi0
Copy link
Author

sabi0 commented Mar 18, 2017

So far I've been thinking of just these two:
Assert.assertContentEquals(CharSequence expected, CharSequence actual)
Assert.assertContentEquals(String message, CharSequence expected, CharSequence actual)

@kcooney
Copy link
Member

kcooney commented Mar 18, 2017

Could you give better examples? I would never write assertEquals("abc", new StringBuilder("abc")) (I would call build() on the builder)

Also I do think DSL based frameworks are often a better fit. In Truth I would write:

assertThat(myCharSequence).hasValue("hello");

Note that hasValue() takes a String so there is no question about what it means for two CharSequence values to be considered equal.

@sabi0
Copy link
Author

sabi0 commented Mar 18, 2017

Here is an example of how it might look in a test:
assertContentEquals("abc", bundle.getCharSequence(Notification.EXTRA_TEXT));

Adding also a HasValue Matcher is a good idea.

@kcooney
Copy link
Member

kcooney commented Mar 18, 2017

Thanks. Since CharSequence doesn't provide a contract for equals() how do you propose we consider two CharSequence values equal? Is it simply that they have the same toString() representation? If so why not make the expected value a String?

@sabi0
Copy link
Author

sabi0 commented Mar 18, 2017

Good point. Though I am hesitant because having expected and actual values with the same type looks cleaner.

And I would also compare the lengths first. To avoid calling toString() unless it's really necessary.

@joseph-mccarthy
Copy link

I would like to take this feature request, please. Could it please be assigned to me. Thank you.

@kcooney
Copy link
Member

kcooney commented Apr 9, 2017

@joseph-mccarthy GitHub apparently only lets you assign issues to someone on the project's team. But we almost never assign issues anyway. Feel free to submit a pull request after reading https://github.com/junit-team/junit4/blob/master/CONTRIBUTING.md

@btrajkovski
Copy link

btrajkovski commented Oct 1, 2017

@kcooney I have question about this issue, is it still up for grabs?
I see that it is linked in PR #1439, but that PR is in request change for quite a long time. Can I take that as a basis for a new PR and go over the review comments?

@kcooney
Copy link
Member

kcooney commented Oct 1, 2017

@joseph-mccarthy are you still working on this?

btrajkovski added a commit to btrajkovski/junit4 that referenced this issue Oct 4, 2017
@btrajkovski
Copy link

@kcooney I am assuming that @joseph-mccarthy is no longer working on this and I've opened a PR with implementation.

@ShivamPandey00
Copy link

is anyone working on this issue ?
I think i can do this feature request. @sabi0 @kcooney

@kcooney
Copy link
Member

kcooney commented Feb 27, 2023

@coderdeadpool JUnit 4.x is in "maintenance mode". Unless there's a critical bug or security issue there likely won't be a release in the foreseeable future, so any work you might do for this issue may not be released for a long time.

If anyone is interested in this functionality, I suggest either 1) using a more robust assertion framework with a fluent API (like Truth or Fest) or 2) exploring what JUnit 5 has to offer (you should be able to use most JUnit 5 assertion APIs without migrating your tests to JUnit 5).

I'm going to close this issue and the related PRs. Feel free to comment if the above suggestions don't work for you.

@kcooney kcooney closed this as completed Feb 27, 2023
@kcooney kcooney reopened this Feb 27, 2023
@kcooney kcooney closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants