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

Introduce assertThatCharSequence to disambiguate Groovy's GString #2791

Closed
scordio opened this issue Sep 27, 2022 · 11 comments · Fixed by #3132
Closed

Introduce assertThatCharSequence to disambiguate Groovy's GString #2791

scordio opened this issue Sep 27, 2022 · 11 comments · Fixed by #3132
Assignees
Labels
language: Groovy An issue related to using AssertJ with Groovy status: ideal for contribution An issue that a contributor can help us with type: improvement A general improvement

Comments

@scordio
Copy link
Member

scordio commented Sep 27, 2022

Feature summary

Currently, Groovy's interpolated expressions cannot be a direct input of assertThat as groovy.lang.GString implements both Comparable and CharSequence.

We might add assertThatCharSequence(CharSequence) / assumeThatCharSequence(CharSequence) and the corresponding BDD variants to disambiguate these calls.

Example

This should compile:

  @Test
  void should_accept_interpolated_expressions() {
    // GIVEN
    def foo = "foo"
    def actual = /.*${foo}.*/
    // WHEN/THEN
    assertThatCharSequence(actual).isEqualTo(".*foo.*")
  }

Implementation notes:

  • The implementation can delegate to the existing assertThat(CharSequence)
  • The new method should also be added to Java6 classes whenever possible. Assertions_sync and Java6Assertions_sync can help figure out where the gaps are.

Workaround

Use toString:

  @Test
  void should_accept_interpolated_expressions() {
    // GIVEN
    def foo = "foo"
    def actual = /.*${foo}.*/
    // WHEN/THEN
    assertThat(actual.toString()).isEqualTo(".*foo.*")
  }
@scordio scordio added language: Groovy An issue related to using AssertJ with Groovy Hackergarten An issue reserved for Hackergarten events (https://www.hackergarten.net/) type: improvement A general improvement labels Sep 27, 2022
@scordio scordio removed the Hackergarten An issue reserved for Hackergarten events (https://www.hackergarten.net/) label Oct 9, 2022
@techrajdeep
Copy link

Hey @scordio, I am looking for an issue for my first contribution . Can you help me here. Can I take on this .

@scordio
Copy link
Member Author

scordio commented Oct 9, 2022

Hi @techrajdeep, sure, this issue is up for grabs. Anything where you'd need clarification?

@scordio scordio added the status: ideal for contribution An issue that a contributor can help us with label Oct 9, 2022
@techrajdeep
Copy link

@scordio Could you please assign it to me

@debanjanc01
Copy link

Hey @scordio! Has this issue been fixed? I don't see a PR for it. If this is still up for grabs, I'd like to work on this.

@scordio
Copy link
Member Author

scordio commented Nov 18, 2022

Sure and thank you, @debanjanc01!

@scordio scordio assigned debanjanc01 and unassigned techrajdeep Nov 18, 2022
@debanjanc01
Copy link

Hey @scordio is there an existing test case that tests this out as you have mentioned in the example for should_accept_interpolated_expressions? Or shall I add the test case to Assertions_assertThat_with_Groovy_strings_Test.groovy?

@debanjanc01
Copy link

Hey @scordio, the problem with groovy.lang.GString is the way it implements the equals(Object) method. It expects that the other object is also an instance of groovy.lang.GString.

In the test case mentioned, assertThatCharSequence(actual).isEqualTo(".*foo.*") will lead to assertion of groovy.lang.GString against java.lang.String. On delegating to the existing assertThat(CharSequence) this would fail as internally CharSequenceAssert#isEqualTo(Object) would call StandardComparisonStrategy#areEqual(Object, Object).

I wanted to know if you have any suggestions to take this forward. This is to understand your preferences and weigh out the different options before proceeding.

@scordio
Copy link
Member Author

scordio commented Nov 19, 2022

Thanks for looking into this, @debanjanc01!

I see the problem, let me think about it and I'll get back to you.

@scordio scordio added the status: pending investigation An issue that needs additional investigation label Nov 19, 2022
@debanjanc01
Copy link

Hey @scordio , wanted to check if you've had the time to look into this.

@scordio
Copy link
Member Author

scordio commented Jul 25, 2023

Hi @debanjanc01, thanks for the ping and apologies for the terribly late feedback 😅

Hey @scordio is there an existing test case that tests this out as you have mentioned in the example for should_accept_interpolated_expressions? Or shall I add the test case to Assertions_assertThat_with_Groovy_strings_Test.groovy?

There is no existing test case at the moment and Assertions_assertThat_with_Groovy_strings_Test.groovy is definitely the right place for a new one.

Hey @scordio, the problem with groovy.lang.GString is the way it implements the equals(Object) method. It expects that the other object is also an instance of groovy.lang.GString.

After checking the topic again and looking at the Groovy sources, the test case I mentioned was definitely wrong. Instead, we should probably target something like the following:

  @Test
  void should_accept_interpolated_expressions() {
    // GIVEN
    def foo = "foo"
    def actual = /.*${foo}.*/
    // WHEN/THEN
    assertThatCharSequence(actual).isEqualTo(/.*${foo}.*/)
  }

I would still delegate to assertThat(CharSequence). As GString::equals would return false with a String input, I don't see anything wrong if isEqualTo would fail.

Would you still have time to pursue this topic? Otherwise, I'm happy to take it over.

@scordio scordio removed the status: pending investigation An issue that needs additional investigation label Jul 25, 2023
@debanjanc01
Copy link

Hey @scordio thanks for taking the time out to revisit this. Let me check this and get back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language: Groovy An issue related to using AssertJ with Groovy status: ideal for contribution An issue that a contributor can help us with type: improvement A general improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants