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

Changed "eventually" usage example to reflect asynchronous logic #990

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dlehammer
Copy link

@dlehammer dlehammer commented Apr 12, 2019

I've attempted to make the usage example more transparent, as I've lost track on how many times I've been asked why the "machine" variable isn't (magically) updated by Spock.
And I attempt to explain that the original Machine example seems to be thread-based.

Hopefully this change aids those who use PollingConditions to test asynchronous logic.


This change is Reviewable

I've attempted to make the usage example more transparent, as I've lost track on how many times I've been asked why the "machine" variable isn't (magically) updated by Spock.
And I attempt to explain that the original Machine example seems to be thread-based.

Hopefully this change aids those who use PollingConditions to test asynchronous logic.
@leonard84
Copy link
Member

What makes the new example more clear in your opinion?

@dlehammer
Copy link
Author

dlehammer commented Jan 22, 2020

I was trying to make the current example more accessible, while trying to be concise.
The idea was to intuitively illustrate that eventually doesn't (implicitly/magically) update the state that's attempted verified for a time period.

Instead the programmer must either

  • Explicitly get a new engine reading to evaluate via a getEngineReading() method every time the eventually-block is executed. (new example)
  • Implicitly await some logic outside the eventually-block updates the machine state, ex. thread-based asynchronous logic, so evaluating the state a later point in time might yield a different result. (current example)

Coming back to this PR I can see that perhaps it's not transparent that I'm assuming the reader will understand that there's a getEngineReading() method, hiding behind bean-notation, that provides the latest state.

Any suggestions?

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #990 into master will decrease coverage by 1.97%.
The diff coverage is 84.38%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #990      +/-   ##
============================================
- Coverage     75.99%   74.01%   -1.98%     
+ Complexity     3544     3432     -112     
============================================
  Files           377      382       +5     
  Lines         10788    10612     -176     
  Branches       1374     1295      -79     
============================================
- Hits           8198     7855     -343     
- Misses         2109     2296     +187     
+ Partials        481      461      -20
Impacted Files Coverage Δ Complexity Δ
...g/spockframework/junit4/AbstractRuleExtension.java 86.66% <ø> (ø) 10 <0> (?)
.../java/org/spockframework/runtime/SpockRuntime.java 71.11% <ø> (+2.03%) 20 <0> (-1) ⬇️
...n/java/spock/util/concurrent/BlockingVariable.java 93.75% <ø> (ø) 6 <0> (ø) ⬇️
...kframework/spring/mock/SpockMockPostprocessor.java 85.03% <ø> (+1.03%) 41 <0> (-1) ⬇️
...in/java/spock/util/concurrent/AsyncConditions.java 93.54% <ø> (ø) 9 <0> (ø) ⬇️
...pock-core/src/main/java/spock/mock/MockingApi.java 2.94% <ø> (ø) 2 <0> (ø) ⬇️
spock-core/src/main/java/spock/lang/Retry.java 100% <ø> (ø) 0 <0> (ø) ⬇️
.../spockframework/report/log/ReportLogExtension.java 100% <ø> (+28.57%) 1 <0> (-7) ⬇️
...spockframework/runtime/SpockComparisonFailure.java 83.33% <ø> (ø) 3 <0> (ø) ⬇️
...spockframework/junit4/AbstractRuleInterceptor.java 85.71% <ø> (ø) 4 <0> (?)
... and 121 more

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 15e3169...3cfc27b. Read the comment docs.

@Vampire
Copy link
Member

Vampire commented Jan 23, 2020

I also don't understand how the new example is any better than the old example.
What does why the "machine" variable isn't (magically) updated mean?
And how would the new example be clearer, it still uses the machine variable, so wouldn't the same question come up?

@dlehammer
Copy link
Author

Hi @Vampire,

What does why the "machine" variable isn't (magically) updated mean?

That's the question I so far always have to explain to my fellow developers, that's stumped by the current machine example vs. the actual behavior when eventually is introduced in a Spock specification.

I've seen a lot of code like the example below from new developers introduced to eventually via the documentation.

...
when:
	def result = myService.getAsyncResult()
then:
	pollingConditions.eventually {
		result.value == expected
	}
...

The above example ends up looping over the state from the initial invocation, and hence never succeeds.

In-order to get the desired behavior instead, I've approached it as follows.

...
expect:
	pollingConditions.eventually {
		def result = myService.getAsyncResult()
		result.value == expected
	}
...

I also don't understand how the new example is any better than the old example.
..
And how would the new example be clearer, it still uses the machine variable, so wouldn't the same question come up?

Well, I was hoping it was possible to collaborate with the maintainers/community in-order to distill an example that doesn't need further explanation to an inexperienced Spock developer.
Ie. how do we convey the power of eventually while simultaneously communicating what's expected by a new developer attempting to utilize eventually.

The current commit is my initial attempt from last year, I'm happy to update it based on suggestions.

@Vampire
Copy link
Member

Vampire commented Jan 24, 2020

Well, this depends heavily on how the domain objects work.
If Machine#getValue() changes what it returns once the asyn operation finished, the first example is perfectly fine.

If this is not the case and the first example is used, it imho only shows that the respective developer did not understand how the asynchronous operation works actually.

I don't think that the second example is any clearer or more correct, especially without information on how the domain object is actually working.
So I'm with Leonard, that the new example is in no way any more helpful and I don't think that a simple example change can introduce the clarity you are after without explicitly describing how the domain objects behave.

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.

3 participants