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 VerifyOnlyElementInFlux Refaster rule #617

Merged
merged 5 commits into from
Aug 14, 2023

Conversation

giovannizotta
Copy link
Contributor

@giovannizotta giovannizotta commented May 7, 2023

When verifying a Flux only has one element, avoid collecting to list.

Notes:

  1. The @BeforeTemplate is quite big, can it be a problem? I don't know if/how it can be reduced in size;
  2. I am pretty sure the name of the rule is not the best, please suggest one that follows existing standards if you have better ideas;
  3. It could be extended to toImmutableSet as well (see comment)
  4. I used flux.collect(toImmutableList()) instead of flux.collectList() because of the rule FluxCollectToImmutableList;
  5. How does it work if there is a match for this rule, but with flux.collectList()? Will error-prone perform "two passes" and first move flux.collectList() to flux.collect(toImmutableList()) and then apply my rule?

@github-actions
Copy link

github-actions bot commented May 7, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Nice draft, in the small hours of the day ;)

@giovannizotta giovannizotta marked this pull request as ready for review May 8, 2023 07:43
@giovannizotta
Copy link
Contributor Author

Nice draft, in the small hours of the day ;)

The best ones, right? 😄

@giovannizotta
Copy link
Contributor Author

giovannizotta commented May 8, 2023

I wanted to extend the rule by also accepting inputs that match flux.collect(toImmutableSet()), but it doesn't work because of different semantics:

  1. In @BeforeTemplate, we would be testing if the flux has only one unique element while allowing to have that one unique element multiple times.
  2. In AfterTemplate we would be testing if the flux has only one element;

But (!) while I was trying to make the rule work, I found a very weird situation that I cannot explain.

The first version of the @BeforeTemplate worked well:

@BeforeTemplate
Duration before(Flux<T> flux, T object) {
  return flux.collect(toImmutableList())
      .as(StepVerifier::create)
      .assertNext(collection -> assertThat(collection).containsExactly(object))
      .verifyComplete();
}

Alongside flux.collect(toImmutableList()), I wanted to add support for flux.collect(toImmutableSet()), so I wrote the following:

@BeforeTemplate
Duration before(Flux<T> flux, T object) {
  return Refaster.anyOf(flux.collect(toImmutableList()), flux.collect(toImmutableSet()))
      .as(StepVerifier::create)
      .assertNext(collection -> assertThat(collection).containsExactly(object))
      .verifyComplete();
}

I am not sure how Refaster#anyOf works, but the following is very weird. Let's say I have the following test:

ImmutableSet<Duration> testVerifyOnlyElementInFlux() {
  return ImmutableSet.of(
      Flux.just(1)
          .collect(toImmutableList())
          .as(StepVerifier::create)
          .assertNext(list -> assertThat(list).containsExactly(1))
          .verifyComplete(),
      Flux.just(2)
          .collect(toImmutableSet())
          .as(StepVerifier::create)
          .assertNext(set -> assertThat(set).containsExactly(2))
          .verifyComplete());
}

I would expect the result to be:

ImmutableSet<Duration> testVerifyOnlyElementInFlux() {
  return ImmutableSet.of(
      Flux.just(1).as(StepVerifier::create).expectNext(1).verifyComplete(),
      Flux.just(2).as(StepVerifier::create).expectNext(2).verifyComplete());
}

The fun part? Now the rule does not match flux.collect(toImmutablelist()), but instead it matches flux.collect(toImmutableSet()) (notice that the second test succeded):

diff (-expected +actual):
    @@ -438,7 +438,11 @@
     
       ImmutableSet<Duration> testVerifyOnlyElementInFlux() {
         return ImmutableSet.of(
    -        Flux.just(1).as(StepVerifier::create).expectNext(1).verifyComplete(),
    +        Flux.just(1)
    +            .collect(toImmutableList())
    +            .as(StepVerifier::create)
    +            .assertNext(list -> assertThat(list).containsExactly(1))
    +            .verifyComplete(),
             Flux.just(2).as(StepVerifier::create).expectNext(2).verifyComplete());
       }
     }

Am I missing something? Am I using Refaster#anyOf in a wrong way? What kind of sorcery is this?

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

  • The @BeforeTemplate is quite big, can it be a problem? I don't know if/how it can be reduced in size;

I proposed a change that makes it one line smaller. In general, this is not really a problem as we are matching a bigger statement in this case.

  • I am pretty sure the name of the rule is not the best, please suggest one that follows existing standards if you have better ideas;

I applied our future naming algorithm which is based on the content of the after template.

  • It could be extended to toImmutableSet as well (see comment)
    I used flux.collect(toImmutableList()) instead of flux.collectList() because of the rule FluxCollectToImmutableList;

Yes that is a good idea. I pushed some changes to make it work. I also tried it with Refaster#anyOf but it didn't really work that well 🤔. With the more specific before template it seems better to have two different before templates because of the Immutable{List,Set}<T> in the return types.

How does it work if there is a match for this rule, but with flux.collectList()? Will error-prone perform "two passes" and first move flux.collectList() to flux.collect(toImmutableList()) and then apply my rule?

Yes Error Prone will fix it in two runs. That's why our internal patch.sh script runs again when changes are introduced by Error Prone.

Added a commit with some changes 😄, thanks for opening the PR 🚀 !

@@ -435,4 +435,8 @@ Duration testStepVerifierLastStepVerifyErrorMessage() {
Duration testStepVerifierLastStepVerifyTimeout() {
return StepVerifier.create(Mono.empty()).verifyTimeout(Duration.ZERO);
}

Duration testVerifyOnlyElementInFlux() {
return Flux.just(1).as(StepVerifier::create).expectNext(1).verifyComplete();
Copy link
Member

Choose a reason for hiding this comment

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

Let's use distinct values in the tests :).


@AfterTemplate
Duration after(Flux<T> flux, T object) {
return flux.as(StepVerifier::create).expectNext(object).verifyComplete();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return flux.as(StepVerifier::create).expectNext(object).verifyComplete();
return flux.as(StepVerifier::create).expectNext(object);

Besides the fact that the return types are now slightly different we can drop the verifyComplete part.
Assuming the tests pass, we can do this rewrite as we test that the list literally contains one object. Strictly speaking this is not a behavior preserving change because of the updated return type. The verifyComplete should be able to handle it though.

As a result we should also move this up as it belongs with the other StepVerifier.Step return type rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should've replied to this comment :) #617 (comment)

@rickie
Copy link
Member

rickie commented May 9, 2023

Am I missing something? Am I using Refaster#anyOf in a wrong way? What kind of sorcery is this?

Hmm, I tried looking into this and combining them but had the same issue.

As before template I had the following:

    @BeforeTemplate
    StepVerifier.Step<? extends ImmutableCollection<T>> before(Flux<T> flux, T object) {
      return Refaster.anyOf(flux.collect(toImmutableList()), flux.collect(toImmutableSet()))
          .as(StepVerifier::create)
          .assertNext(list -> assertThat(list).containsExactly(object));
    }

Had the same problem, requires some more brain cycles at a different point in time :). Now have to focus on something else.

(Note to self: didn't check the Javadoc yet)

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

github-actions bot commented May 9, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@giovannizotta
Copy link
Contributor Author

Hey @rickie thanks for the review! I won't have much time in the next days, but I'll pick this up.

Just one thing about making it one line smaller: in my understanding, if we don't check for verifyComplete() we could have a situation where the input does not do verifyComplete(), and the output would give a warning Unfinished step verifier.

I'll come back to this asap, probably in the weekend 😄

Screenshot 2023-05-11 at 08 20 17

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Had a quick peek; nice rule!

Just one thing about making it one line smaller: in my understanding, if we don't check for verifyComplete() we could have a situation where the input does not do verifyComplete(), and the output would give a warning Unfinished step verifier.

As-is the rule will match both with and without verifyComplete(), meaning it's more generic; a property we generally like. It's okay if a rule also matches code that is "otherwise wrong" 😄

Comment on lines 1309 to 1328
return flux.collect(toImmutableSet())
.as(StepVerifier::create)
.assertNext(set -> assertThat(set).containsExactly(object));
Copy link
Member

Choose a reason for hiding this comment

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

This is not equivalent to the code below, since toImmutableSet() drops duplicates. We could add an XXX comment, or drop this case 🤔.

(If we keep it: Should indeed have a closer look at that Refaster#anyOf issue.)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that would indeed be the best option. Might find some time for that later this week, but not sure.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I think fixing this issue requires creating a Refaster rule in Error Prone and debugging why this specific Refaster#anyOf usage doesn't work. That sounds like a bit more work. I'd suggest to drop the toImmutableSet() and create an XXX and pick up that debugging later. WDYT? @Stephan202

@@ -1295,6 +1295,28 @@ StepVerifier.Step<T> after(StepVerifier.Step<T> step, T object) {
}
}

/** Avoid collecting when verifying only the given element is in the given {@link Flux}. */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Avoid collecting when verifying only the given element is in the given {@link Flux}. */
/** Avoid value collection when verifying that a {@link Flux} emits exactly one value. */

@rickie rickie added this to the 0.12.0 milestone May 16, 2023
@giovannizotta
Copy link
Contributor Author

As-is the rule will match both with and without verifyComplete()

Isn't this a problem? Let's say the original code is:

Flux.just(1)
    .collect(toImmutableList())
    .as(StepVerifier::create)
    .assertNext(list -> assertThat(list).containsExactly(1));

This code is correct and does not give warnings. We are sure the flux is completed because we collected to list and then verified the only element in the list was the expected one.
In the new code:

Flux.just(1).as(StepVerifier::create).expectNext(1);

We are not verifying if the flux is completed, which gives the warning Unfinished StepVerifier. So we are turning code that compiles into code that gives a warning, that does not seem right to me?

@Stephan202
Copy link
Member

This code is correct and does not give warnings. We are sure the flux is completed because we collected to list and then verified the only element in the list was the expected one.

That code might not yield a warning, but that's just a limitation of IDEA; it is just as incorrect. Neither variant verifies anything 😬.

The following check should fail, but doesn't (because there's no verification):

Flux.just(2)
    .collect(toImmutableList())
    .as(StepVerifier::create)
    .assertNext(list -> assertThat(list).containsExactly(1));

@giovannizotta
Copy link
Contributor Author

giovannizotta commented Jun 18, 2023

I am sorry for not coming back at this for more than a month, it has been really busy lately 😄 I hope there's no rush.

To be honest, I still don't understand. Take the following snippet:

@BeforeTemplate
StepVerifier.Step<ImmutableList<T>> before(Flux<T> flux, T object) {
  return flux.collect(toImmutableList())
      .as(StepVerifier::create)
      .assertNext(list -> assertThat(list).containsExactly(object));
}

@AfterTemplate
StepVerifier.Step<T> after(Flux<T> flux, T object) {
  return flux.as(StepVerifier::create).expectNext(object);
}

The code in the @BeforeTemplate collects the flux in a Mono<ImmutableList<T>> and checks that the list contains one and only one element, and checks if it is specifically the one we're looking for.
The code in the @AfterTemplate is checking if the first element of the flux is the one we're looking for. However, it does not have any check on the length of the flux.
To me without the verifyComplete() on the @AfterTemplate, these 2 templates are testing different things and therefore not a great fit for error-prone, unless I am missing something (which is likely 😄).
Am I missing something? Please let me know. No rush though, and again, sorry for the delay 😅

@giovannizotta giovannizotta force-pushed the giovannizotta/verifyonlyelementinflux branch from 5cad15f to 200aafc Compare June 18, 2023 17:33
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 modified the milestones: 0.12.0, 0.13.0 Jun 21, 2023
@mohamedsamehsalah
Copy link
Contributor

mohamedsamehsalah commented Jul 3, 2023

@giovannizotta Am I missing something? Please let me know. No rush though, and again, sorry for the delay 😅

Hello, just passing through 👀

I think the confusion is that

  return flux.collect(toImmutableList())
      .as(StepVerifier::create)
      .assertNext(list -> assertThat(list).containsExactly(object));

is also checking for one and only one element (Since we only have one assertNext call)

As far as I can tell, the @AfterTemplate is the less verbose method to check for this.

@giovannizotta
Copy link
Contributor Author

giovannizotta commented Jul 3, 2023

Hey @mohamedsamehsalah thanks for passing by!
The issue I see is the opposite:

  • the @BeforeTemplate is checking that the flux has one and only one element, and that the element corresponds to the one we're looking for
  • the @AfterTemplate is checking that the flux has at least one element, and that the first element corresponds to the one we're looking for. However no checks that the flux contains only one element.

So they are testing two different things 🤔

@rickie
Copy link
Member

rickie commented Jul 6, 2023

Thanks for thoroughly thinking about this and explaining your thought process, I like it 😄!

One important assumption we are making here, which should be true for 99% of the cases, is that someone is using something like verifyComplete() after the assertNext. As Stephan mentions here, while the code doesn't yield a warning, it doesn't verify anything.

Having the @BeforeTemplate without .verifyComplete() makes it more generic, which should also be fine in most cases. If it appears that this is not the case, we can always decide to make the rule more strict. For example, by creating a Matcher (examples) that verifies that the assertNext is followed by a verifyComplete.
Another option what would maybe even be better is creating another check (probably a BugPattern would be better for this) that enforces that an assertNext is followed by an verifyComplete. That would even be better and make this Refaster rule 100% valid 😉. (We can probably make that rule more generic even).

these 2 templates are testing different things

So together with the knowledge that an assertNext is not verifying anything, and therefore the assumption that we have an verifyComplete(), we are actually doing a correct replacement.

Let's consider the following failing test:

 Flux.just(1, 2)
         .collect(toImmutableList())
         .as(StepVerifier::create)
         .assertNext(list -> assertThat(list).containsExactly(1))
         .verifyComplete();

Would correctly be rewritten to:

   Flux.just(1, 2)
        .as(StepVerifier::create)
        .expectNext(1)
        .verifyComplete();

Which would fail with:

java.lang.AssertionError: expectation "expectComplete" failed (expected: onComplete(); actual: onNext(2))

Here a quick (not fullproof) GitHub search on Picnic code that shows we are in all cases verifying.

If I'm missing anything in my explanation, please let me know 😄, happy to elaborate further!

@giovannizotta giovannizotta force-pushed the giovannizotta/verifyonlyelementinflux branch from 200aafc to 36444a3 Compare July 6, 2023 14:58
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@giovannizotta giovannizotta force-pushed the giovannizotta/verifyonlyelementinflux branch from 058a070 to 8492c5a Compare July 14, 2023 21:03
@giovannizotta
Copy link
Contributor Author

@rickie thanks a lot for the amazing explanation! Also, for the offline discussion 😄
Now it's very clear: I was confused on how the step verifier works, as I am really a reactive noobie. But the explanation makes a lot of sense 😄.

To finish up the PR, I decided to move on with the suggestion to remove the toImmutableSet() rule because of the different semantics. Also, at some point I would like to look into building the matcher you mentioned, but let's keep that for the future 🙂.

Just a question about the test framework. I tried minimising the test by removing the verifyComplete(), but that leads to having a different return type for the function in the before and after template.
The test framework does not substitute the return type, so if we give the proper return type to the After template we end up with a mismatch:

-  StepVerifier.Step<Integer> testFluxAsStepVerifierExpectNext() {
+  StepVerifier.Step<ImmutableList<Integer>> testFluxAsStepVerifierExpectNext() {

And if we give StepVerifier.Step<ImmutableList<Integer>> as a return type to the @After:

incompatible types: reactor.test.StepVerifier.Step<java.lang.Integer> cannot be converted to reactor.test.StepVerifier.Step<com.google.common.collect.ImmutableList<java.lang.Integer>>

So I decided to just stick to also having a verifyComplete() in the test to have a Duration as a return type in both cases.

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

1 similar comment
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@giovannizotta giovannizotta force-pushed the giovannizotta/verifyonlyelementinflux branch from 8492c5a to 6536c11 Compare July 22, 2023 13:43
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@giovannizotta giovannizotta force-pushed the giovannizotta/verifyonlyelementinflux branch from 6536c11 to db51011 Compare August 6, 2023 17:05
@github-actions
Copy link

github-actions bot commented Aug 6, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Let's indeed not consider the toImmutableSet for now.

@rickie rickie force-pushed the giovannizotta/verifyonlyelementinflux branch from db51011 to 3da3627 Compare August 14, 2023 06:25
@rickie
Copy link
Member

rickie commented Aug 14, 2023

@giovannizotta Your reasoning about the tests is correct, we can use the verifyComplete there :).

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 force-pushed the giovannizotta/verifyonlyelementinflux branch from 3da3627 to b198db4 Compare August 14, 2023 09:20
@Stephan202 Stephan202 force-pushed the giovannizotta/verifyonlyelementinflux branch from b198db4 to 1d52d89 Compare August 14, 2023 09:20
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Nice discussions!

Rebased and added a commit. Suggested commit message:

Introduce `FluxAsStepVerifierExpectNext` Refaster rule (#617)

static final class FluxAsStepVerifierExpectNext<T> {
@BeforeTemplate
StepVerifier.Step<ImmutableList<T>> before(Flux<T> flux, T object) {
return flux.collect(toImmutableList())
Copy link
Member

Choose a reason for hiding this comment

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

Though it comes with some caveats, we can also match other list-producing collectors here, such as toCollection(ArrayList::new).

.collect(toImmutableList())
.as(StepVerifier::create)
.assertNext(list -> assertThat(list).containsExactly(2))
.verifyComplete();
Copy link
Member

Choose a reason for hiding this comment

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

This .verifyComplete() can be omitted (which hides that we do change the return type here :).)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, we can do the <?> as return type indeed.

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

1 similar comment
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Comment on lines 425 to 435
ImmutableSet<StepVerifier.Step<?>> testFluxAsStepVerifierExpectNext() {
return ImmutableSet.of(
Flux.just(1)
.collect(toImmutableList())
.as(StepVerifier::create)
.assertNext(list -> assertThat(list).containsExactly(2)),
Flux.just(3)
.collect(toCollection(ArrayList::new))
.as(StepVerifier::create)
.assertNext(list -> assertThat(list).containsExactly(4)));
}
Copy link
Member

Choose a reason for hiding this comment

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

After pushing this I realized that perhaps for rule one test case would suffice, or at least be more consistent with what we do elsewhere. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I'll add a commit to drop the second test case.

@rickie
Copy link
Member

rickie commented Aug 14, 2023

Will merge once 🍏.

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rickie rickie merged commit 7fb0e55 into master Aug 14, 2023
@rickie rickie deleted the giovannizotta/verifyonlyelementinflux branch August 14, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants