Add BlockListener support...

[leonard84]

This feature allows extension authors to register a IBlockListener for
a feature to observe the execution of a feature in more detail.
This surfaces some of Spock's idiosyncrasies, for example interaction
assertions are actually setup right before entering the preceding
when-block as well as being evaluated on leaving the when-block
before actually entering the then-block.

The only valid block description is a constant String, although some
users mistakenly try to use a dynamic GString. Using anything other
than a String, will be treated as a separate statement and thus ignored.

fixes #538
fixes #111

[codecov]

Codecov Report

Attention: Patch coverage is 79.47020% with 31 lines in your changes missing coverage. Please review.

Project coverage is 81.79%. Comparing base (2c7db77) to head (a8347ad).
Report is 177 commits behind head on master.

Files with missing lines	Patch %	Lines
...rg/spockframework/runtime/DataIteratorFactory.java	33.33%	10 Missing ⚠️
.../java/org/spockframework/runtime/ErrorContext.java	63.15%	7 Missing ⚠️
.../spockframework/compiler/model/BlockParseInfo.java	66.66%	4 Missing ⚠️
...ava/org/spockframework/compiler/SpecAnnotator.java	66.66%	1 Missing and 1 partial ⚠️
...ockframework/runtime/extension/IBlockListener.java	0.00%	2 Missing ⚠️
...va/org/spockframework/runtime/model/BlockInfo.java	66.66%	2 Missing ⚠️
...java/org/spockframework/compiler/SpecRewriter.java	97.05%	0 Missing and 1 partial ⚠️
.../java/org/spockframework/compiler/model/Block.java	83.33%	1 Missing ⚠️
...org/spockframework/runtime/PlatformSpecRunner.java	75.00%	1 Missing ⚠️
...va/org/spockframework/runtime/model/ErrorInfo.java	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1575      +/-   ##
============================================
+ Coverage     80.44%   81.79%   +1.34%     
- Complexity     4337     4642     +305     
============================================
  Files           441      450       +9     
  Lines         13534    14582    +1048     
  Branches       1707     1839     +132     
============================================
+ Hits          10888    11927    +1039     
+ Misses         2008     1975      -33     
- Partials        638      680      +42     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kriegaex]

Hi @leonard84. I cannot perform a formal code review, because I am not a committer, but I hope that during the weekend I can find a small time slice to build and play around with it. I simply wanted to say thanks in advance for taking care of this feature request. It has not gone unnoticed.

Quick question: Are you planning to add more commits to this PR? Code changes? User manual? I am just asking, not demanding anything. I simply do not want to start testing too early. As for the user manual, I can of course take a look at the unit tests and take it from there. But that might not be true for all future extension developers, I am just speaking for myself.

[leonard84]

@kriegaex I'll write some documentation for it, but I mainly wanted to get feedback on the feature first, like the one from @szpak concerning the usability.

[leonard84]

💬 this won't be called, fixing this makes for some awkward interactions between the individual AST transformations,

[leonard84]

I tried moving the blockListener logic where the blocks are written back in SpecRewriter#L388, however this didn't work for some cases, e.g. cleanup as it already replaced all Blocks with an anonymous block.

[szpak]

As this PR seems a little bit stalled, I pushed the code I initially created ~6 months ago as a PoC for the BlockListener support. It a very raw version (with a lot of diagnostic stuff to be amended in the future), but functional in some basic scenarios. The JPA session, where requested, is flushed after the when block.

https://github.com/szpak/spock-jpa-flush-enforcer/tree/preview1

I would like to awake discussion about future shape of this PR.

@leonard84 @kriegaex WDYT?

[leonard84]

It mostly hangs on the problems that adding the exit listener introduces #1575 (comment)

[leonard84]

I think I've fixed the issues, but I need another fresh set of eyes to verify that everything looks good, I've stared at too many snapshots already.

[leonard84]

@AndreasTu, as I mentioned earlier, I'll write some documentation and polish it later. Thanks for your comments in any case.

However, I'm looking for a review of the correctness of the implementation and generated code and general usability.

[szpak]

I've switched my experimental (and heavily work-in-progress) extension to the latest version and it still works as expected without any modification. Nice.

I will think about the possible corner cases, I might encounter there.

Btw, I wonder, if it is still a recommended way to decide if the block listener was intended for the current iteration?

[Vampire]

Some things missing in multiple places:

• license headers
• JavaDoc
• documentation
• @Beta
• @since

[Vampire]

Btw. could we also support where (and upcoming filter) blocks in a sensible way?

[leonard84]

Btw. could we also support where (and upcoming filter) blocks in a sensible way?

I don't think it would be intuitive or really helpful.
They run outside the normal iteration.
When would you enter or leave the block?

• only once when creating the data provider
• on every data iterator's iteration?

However, you are most familiar with that part of the code.

[Vampire]

When would you enter or leave the block?

No idea. :-D
I guess multiple times.
Maybe with an additional phase enum.
One for creating the data iterators.
One for getting the next set of data variable values.

But we can also start without and see where and how need arises.
But then it needs time to land in production version with our release cadence. :-D

[leonard84]

Forgot to publish my responses 😅

[leonard84]

Btw, I wonder, if it is still a recommended way to decide if the block listener was intended for the current iteration?

@szpak, can you motivate this use case more? At first glance, I'd try to register a block listener that can handle all iterations instead of one per iteration.

[szpak]

@szpak, can you motivate this use case more? At first glance, I'd try to register a block listener that can handle all iterations instead of one per iteration.

Hmm, I think the main problem was to get the invocation instance to obtain the current field instance. As a result, I put IBlockListener inside AbstractMethodInterceptor which provides invocation. Would you propose @leonard84 to pass the invocation instance to the listener in any simpler way?

[leonard84]

@szpak, can you motivate this use case more? At first glance, I'd try to register a block listener that can handle all iterations instead of one per iteration.

Hmm, I think the main problem was to get the invocation instance to obtain the current field instance. As a result, I put IBlockListener inside AbstractMethodInterceptor which provides invocation. Would you propose @leonard84 to pass the invocation instance to the listener in any simpler way?

We could easily pass in the current instance. The question is whether we should.

I've also been debating whether I should give access to the current ISpockExecution so that a listener can get an IStore.

[szpak]

Hmm, I think the main problem was to get the invocation instance to obtain the current field instance. As a result, I put IBlockListener inside AbstractMethodInterceptor which provides invocation. Would you propose @leonard84 to pass the invocation instance to the listener in any simpler way?

We could easily pass in the current instance. The question is whether we should.

Definitely that's the good question. What are the design difference between interceptors (which have access to invocation) and listeners (also in work there are intended for)? The first could abort the processing, while listeners should not. Listeners should only perform (external) side effects? What also is different and if invocation in the second case could "complicate" something?

I've also been debating whether I should give access to the current ISpockExecution so that a listener can get an IStore.

Do you see any good cases where it would be necessary for listeners in practice? To read the values set by the other extensions/interceptors?

[Vampire]

lgtm now, thx