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[bot]]

Codecov Report

Attention: Patch coverage is 79.57746% with 29 lines in your changes missing coverage. Please review.

Project coverage is 81.85%. Comparing base (2c7db77) to head (d6f4c9d). Report is 100 commits behind head on master.

Files	Patch %	Lines
...rg/spockframework/runtime/DataIteratorFactory.java	40.00%	9 Missing :warning:
.../java/org/spockframework/runtime/ErrorContext.java	63.15%	7 Missing :warning:
...main/java/org/spockframework/compiler/AstUtil.java	50.00%	2 Missing :warning:
...ava/org/spockframework/compiler/SpecAnnotator.java	33.33%	1 Missing and 1 partial :warning:
...java/org/spockframework/compiler/SpecRewriter.java	94.73%	0 Missing and 2 partials :warning:
...ockframework/runtime/extension/IBlockListener.java	0.00%	2 Missing :warning:
...va/org/spockframework/runtime/model/BlockInfo.java	66.66%	2 Missing :warning:
.../java/org/spockframework/compiler/model/Block.java	83.33%	1 Missing :warning:
...org/spockframework/runtime/PlatformSpecRunner.java	75.00%	1 Missing :warning:
...va/org/spockframework/runtime/model/ErrorInfo.java	75.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1575      +/-   ##
============================================
+ Coverage     80.44%   81.85%   +1.40%     
- Complexity     4337     4603     +266     
============================================
  Files           441      448       +7     
  Lines         13534    14444     +910     
  Branches       1707     1821     +114     
============================================
+ Hits          10888    11823     +935     
+ Misses         2008     1950      -58     
- Partials        638      671      +33     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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.

[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 https://github.com/spockframework/spock/pull/1575#discussion_r1132797320

[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.

[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?

[leonard84]

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?

IBlockListerner is a sibling of IRunListener; both are intended for reporting purposes and not to perform test-relevant side effects.

What you are trying to do (interact with the entity manager) is not in line with the original intention, and I'm still not 100% convinced that it is a good thing. Yet, it would make sense to explore what it would take to support this properly to make an informed decision.

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?

It would make it possible to access information added by a cooperating extension for that purpose.

[szpak]

What you are trying to do (interact with the entity manager) is not in line with the original intention, and I'm still not 100% convinced that it is a good thing.

In that case, maybe it is good to don't have an official support for that 🤷. Probably we don't also want to have a dedicated interceptor for blocks (I'm not sure for what other cases it could be used). In that case the "hack" I used - assuming it is stable - is sufficient for me (and deters potential followers ;-) ). It's the only possibility right now to implement the "flushing extension".

[leonard84]

@szpak FWIW, you can probably get away with using a single instance of each and a ThreadLocal to share the data from the interceptor to the listener.

[leonard84]

@renatoathaydes do you have any comments about the new listeners?

[leonard84]

@Vampire, @szpak, @AndreasTu:

Historically, Spock ignored block labels that were GString expressions, e.g., when: "user ${user.name}" instead, those were kept as standalone GStrings doing mostly nothing, except for expect/then blocks where they would be asserted. This was mainly because the blocks are stored in @BlockMetadata, which only allows constant strings.

Now we have to decide if we want IBlockListener to support GString expressions as labels. It would be a potential breaking change, although the impact should be negligible. The @BlockMetadata would store the verbatim representation, and the blockEntered/blockExited methods would evaluate the GString when constructing the BlockInfo.

    int idx = 0
    given: "given ${idx++}"
    expect: "expect ${idx++}"
    when: "when ${idx++}"
    then: "then ${idx++}"

would end up with idx == 8 as each enter/exit would increment it.

The alternative would be to stay with only constant strings and with the possible optimization to re-use the existing BlockInfo from he FeatureInfo and access it via index, avoiding the construction of new instances for each block* call.

[szpak]

@szpak FWIW, you can probably get away with using a single instance of each and a ThreadLocal to share the data from the interceptor to the listener.

Trying to use ThreadLocal was failing with strange effect at first and in the end, it turned out that my original problem with "wrong iteration" was caused by the fact, I was not removing the block listener added in the iteration interceptor. As a result, in the second iteration 2 listeners were called (and checking the iteration index worked as a workaround)...

In the end, I can use just one block listener without any iteration index checking. Thanks.

https://github.com/szpak/spock-jpa-flush-enforcer/compare/preview2...preview3?expand=1

[leonard84]

@szpak you should just register the blocklister once for the feature where you add the interationInterceptor instead of adding/removing it every time. Otherwise, you'll run into problems in parallel execution mode.

[szpak]

@szpak you should just register the blocklister once for the feature where you add the interationInterceptor instead of adding/removing it every time. Otherwise, you'll run into problems in parallel execution mode.

Thanks, fixed: https://github.com/szpak/spock-jpa-flush-enforcer/commit/bb830e1bc2297b07fab1e7a24edef70d42261b48

[szpak]

Regarding GString in labels, I do not have a strong opinion. It might be useful for (more) fancy reporting, so some people might be delighted. On the other hand, I don't know how my (performance) impact it will have.

It would be a potential breaking change, although the impact should be negligible.

Some people might use it as a informal (and broken) placeholders. However, it should be easy to fix.

Btw, could we reuse BlockInfo if no GString is detected, and create a new instance only if an evaluation is needed?

[leonard84]

Btw, could we reuse BlockInfo if no GString is detected, and create a new instance only if an evaluation is needed?

Maybe, with increased complexity.

[AndreasTu]

I have no real preference in any way for or against the GString support.

How about we go with the existing behavior, and if someone wants the GString support in the future, we can still make the break?

[renatoathaydes]

@renatoathaydes do you have any comments about the new listeners?

I am a bit out of the loop. But this sounds very useful to me.

[leonard84]

@renatoathaydes, please take a look at the new BlockListener and ErrorInfo interfaces.

I guess you are going to be one of the prime users of this feature, so if you have any comments, now would be the best time to address them.

[renatoathaydes]

@leonard84 oh I see. I will try to checkout this code and get spock-reports to use the new feature, I suppose that will let me fix https://github.com/renatoathaydes/spock-reports/issues/242 and maybe even https://github.com/renatoathaydes/spock-reports/issues/89?

[leonard84]

@renatoathaydes, did you have a chance to take a look?