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

Add documentation for concurrency helpers #1173

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

thokari
Copy link

@thokari thokari commented May 31, 2020

This is for #1058 , please let me know if this is what you expect, then I will wrap it up for the other classes.
Also the indentation looks a bit strange for the number hints on the AgeFilter example, I'd fix that as well if you don't mind.


This change is Reviewable

@codecov
Copy link

codecov bot commented May 31, 2020

Codecov Report

Merging #1173 (05e6d3e) into master (b8bfe54) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1173   +/-   ##
=========================================
  Coverage     76.52%   76.52%           
- Complexity     3754     3756    +2     
=========================================
  Files           405      405           
  Lines         11437    11437           
  Branches       1396     1395    -1     
=========================================
  Hits           8752     8752           
  Misses         2197     2197           
  Partials        488      488           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8bfe54...05e6d3e. Read the comment docs.

Copy link
Member

@leonard84 leonard84 left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution, sorry it took so long to review.

docs/utilities.adoc Outdated Show resolved Hide resolved
[[blocking-variables]]
== Evaluating asynchronous variables with `BlockingVariable` and `BlockingVariables`

The two utility classes `BlockingVariable` and `BlockingVariables` are there to help with collecting variables that are
Copy link
Member

Choose a reason for hiding this comment

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

Since Spock 2.0 uses Java 8 BlockingVariable is kind of obsolete as there is CompletableFuture, we are still debating whether to deprecate it or remove it entirely.

Copy link
Author

Choose a reason for hiding this comment

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

And this should be mentioned in the docs?

Copy link
Member

Choose a reason for hiding this comment

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

No, but if the class is going to be deprecated or deleted it does not need to be documented. ;-)

docs/utilities.adoc Outdated Show resolved Hide resolved
<6> call `evaluate` multiple times
<7> call `await` in the end, specifying 2.5 seconds as the timeout

For more information have a look at the test code `spock.util.concurrent.AsyncConditionsSpec`.
Copy link
Member

Choose a reason for hiding this comment

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

As it is rather uncommon to use AsyncConditions in production code, it might be helpful to show its usage via a Mock/Spy as this would be closer to its real world usage.

Copy link
Author

Choose a reason for hiding this comment

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

Production code? I'm not sure I understand, could you elaborate on what you have in mind here?

Copy link
Member

Choose a reason for hiding this comment

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

Well, you somehow need to trigger these conditions.evaluate calls. The current examples show the direct usage in the test code via Thread.start. While that is a nice bare bones example, it does not really help the user to see how he can use that to test his actual production code.
One way, is that you'd define a Spy and then do something like service.foo() >> { conditions.evaluate { ... }; callRealMethod()}.

docs/utilities.adoc Outdated Show resolved Hide resolved
docs/utilities.adoc Outdated Show resolved Hide resolved
docs/utilities.adoc Outdated Show resolved Hide resolved
docs/utilities.adoc Show resolved Hide resolved

when:
Thread.start {
Thread.sleep(500) // <5>
Copy link
Member

Choose a reason for hiding this comment

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

Please use shorter sleeps as this slows down our overall tests, i.e. 100 or less

Copy link
Author

Choose a reason for hiding this comment

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

I made some tests, i.e. repeating the specs ~50 times, and it seemed that (on my machine) the sleeps could go as low as about 10ms before the order of execution for some of the statements would be changing. So I chose 25ms and 50ms respectively in the commit I just made.
Mind that there are also some timeouts in the actual tests, i.e. in the spock.util.concurrent package, which is where I got the numbers from in the first place. I considered lowering them also, but didn't want to without your approval. Let me know, and I will change those numbers also.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the CI servers are slower than most PCs so that is why there is a bit of a margin in the BlockingVariable(s)Spec. It could probably be lowered to 250 without making the tests too flaky on ci.

However, if the time is not really critical to the test it should be as low possible. In these tests it is just used to visualize a delay, the correctness of BlockingVariable is already tested in the other specs.

<6> call `evaluate` multiple times
<7> call `await` in the end, specifying 2.5 seconds as the timeout

For more information have a look at the test code `spock.util.concurrent.AsyncConditionsSpec`.
Copy link
Member

Choose a reason for hiding this comment

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

Well, you somehow need to trigger these conditions.evaluate calls. The current examples show the direct usage in the test code via Thread.start. While that is a nice bare bones example, it does not really help the user to see how he can use that to test his actual production code.
One way, is that you'd define a Spy and then do something like service.foo() >> { conditions.evaluate { ... }; callRealMethod()}.


when:
Thread.start {
Thread.sleep(500) // <5>
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, the CI servers are slower than most PCs so that is why there is a bit of a margin in the BlockingVariable(s)Spec. It could probably be lowered to 250 without making the tests too flaky on ci.

However, if the time is not really critical to the test it should be as low possible. In these tests it is just used to visualize a delay, the correctness of BlockingVariable is already tested in the other specs.

@winne42
Copy link

winne42 commented Apr 25, 2023

@leonard84 , @Vampire , @thokari
Will this ever be merged? Testing concurrency is always hard, insufficient documentation makes it even harder.

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