Add documentation for concurrency helpers #1173
Conversation
Codecov Report
@@ 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.
|
...s/src/test/groovy/org/spockframework/docs/utilities/concurrent/AsyncConditionsDocSpec.groovy
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
And this should be mentioned in the docs?
There was a problem hiding this comment.
No, but if the class is going to be deprecated or deleted it does not need to be documented. ;-)
docs/utilities.adoc
Outdated
| <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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Production code? I'm not sure I understand, could you elaborate on what you have in mind here?
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Please use shorter sleeps as this slows down our overall tests, i.e. 100 or less
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/utilities.adoc
Outdated
| <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`. |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
|
@leonard84 , @Vampire , @thokari |
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
AgeFilterexample, I'd fix that as well if you don't mind.This change is