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 BlockListener support... #1575

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

Conversation

leonard84
Copy link
Member

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
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Attention: Patch coverage is 85.12397% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 81.87%. Comparing base (2c7db77) to head (d9b7e2e).
Report is 97 commits behind head on master.

Current head d9b7e2e differs from pull request most recent head b16eac0

Please upload reports for the commit b16eac0 to get more accurate results.

Files Patch % Lines
...rg/spockframework/runtime/DataIteratorFactory.java 40.00% 9 Missing ⚠️
.../java/org/spockframework/runtime/ErrorContext.java 82.35% 3 Missing ⚠️
...main/java/org/spockframework/compiler/AstUtil.java 50.00% 2 Missing ⚠️
...ockframework/runtime/extension/IBlockListener.java 0.00% 2 Missing ⚠️
...java/org/spockframework/compiler/SpecRewriter.java 97.22% 0 Missing and 1 partial ⚠️
...org/spockframework/runtime/PlatformSpecRunner.java 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1575      +/-   ##
============================================
+ Coverage     80.44%   81.87%   +1.42%     
- Complexity     4337     4599     +262     
============================================
  Files           441      448       +7     
  Lines         13534    14428     +894     
  Branches       1707     1816     +109     
============================================
+ Hits          10888    11813     +925     
+ Misses         2008     1947      -61     
- Partials        638      668      +30     

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

import org.spockframework.runtime.model.IterationInfo;

public interface IBlockListener {
void blockEntered(IterationInfo iterationInfo, BlockInfo blockInfo);
Copy link
Member

@szpak szpak Feb 16, 2023

Choose a reason for hiding this comment

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

Seeing this PR, it brought back our discussion on Slack about a possibility to write a - much desired by me - extension "calling flush in database tests with rollback". I immediately, started to write a PoC which led me to a question, if there are use cases where blockExited() counterpart could be needed?

In the aforementioned example, placing a "flush call" at the end of the when block would be preferred. Otherwise, the extension would need to cover both then and expect blocks, which here should have similar effect (maybe a little more redundant 2x flush with a when..then..then construction).

Calling "something" after the then block, regardless it is the last then block or just the first one in the when..then..when..then sequence? It would be more tricky to handle with just blockEntered.

Do you see any glitches (e.g. with mocks) if blockExited would be also available?

Adding that method later would break backward compatibility and to not force people to implement it, blockExited could even have a default implementation (if blockEntered were preferred).

Update. A temporary mental block... Of course, with a default method, it would be possible to add that second method later. However, if there are no "implementation issues", I propose to have two methods and maybe also create AbstractBlockListener to do not force people to implement both of them, but only thatneeded.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had thought about it and initially decided against it, as it was another method call added to every block.
The other thing I was unsure about was how to deal with exceptions, if we wanted to call blockExited even when an exception happened then that would require wrapping block in yet another try-catch, changing the variable scope.
That is why I ultimately decided against implementing it that way, you still have access to the error and block by using the IRunListener.error method with the newly enhanced currentBlock info.
When using the easy/elegant way to add the blockExited it surfaces some potentially undesirable interactions with other transforms.
Mainly with interaction validation, i.e. leaveScope() and the way thrown() will rewrite the AST, see the generated AST for details.
I've committed the change for review.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't thought about the exception handling in the "blockExited" before, but definitely it complicates the situation. Good that you were able to solve it. I will try to play with it in my PoC extension, but I have other conceptional question before.

The extension is intended to call flush() on auto detected or specified field (e.g. EntityManager) in blockExisted. IBlockListener.blockExited() is called in the right moment, but to get a current state of the field, the IMethodInvocation is needed (for myfieldInfo.readValue(invocation.getInstance())).

Was it intended to add a block listener from a method execution interceptor or I misuse it?

//annotation driven extension class

  @Override
  public void visitSpecAnnotation(EnforceJpaSessionFlush annotation, SpecInfo spec) {

    FieldInfo entityManagerField = ...   //just a simple case, no corner cases

    spec.getFeatures().forEach(methodInfo -> {     //ignore features from super class for a moment
        methodInfo.addInterceptor(new AbstractMethodInterceptor() {

          @Override
          public void interceptFeatureExecution(IMethodInvocation invocation) throws Throwable {  //also IterationExecution
            invocation.getFeature().addBlockListener(new IBlockListener() {
              @Override
              public void blockExited(IterationInfo iterationInfo, BlockInfo blockInfo) {
                if (blockInfo.getKind() == BlockKind.WHEN) { 
                  ...
                  Object entityManager = entityManagerField.readValue(invocation.getInstance());
                  GroovyRuntimeUtil.invokeMethod(entityManager, "flush");
                }
              }
            });
            invocation.proceed();
          }
        });
      });

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, it is not really intended to be used that way, as this will not work properly when you adding a listener to a data-driven feature.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know, I haven't checked the data-driven feature yet. Nevertheless, my use case with calling JPA flush after the when block perfectly matches BlockListener, so maybe you will figure our a way to make it work also with data-driven features :).

Copy link
Member

Choose a reason for hiding this comment

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

Well, we could just add the current instance to the arguments.

That would be some option (assuming it will be easy to get the execution context/instance in DeepBlockRewriter).

Btw, when I replaced interceptFeatureExecution with interceptIterationExecution it seems to work for both regular and data-driven features (originally I used both the interceptors as I thought the interceptIterationExecution is not called for regular features - as IRunListener.beforeIteration(), but it is and I had doubled execution). However, maybe there are some other (yet unknown for me) corner cases.

Copy link
Member Author

@leonard84 leonard84 Mar 15, 2023

Choose a reason for hiding this comment

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

That would be some option (assuming it will be easy to get the execution context/instance in DeepBlockRewriter).

It would be just generating this as parameter.

The more basic question would be, should these block listeners perform any work than can throw an exception, and if so, should we handle it in any way, or just let it propagate?

I was mainly seeing these as tools for reporting, not doing stuff that affects the execution of the spec/feature.

Copy link
Contributor

@kriegaex kriegaex Mar 15, 2023

Choose a reason for hiding this comment

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

My maybe not very helpful thoughts about this are: You can expect users to try to (ab)use the new block listeners in all ways possible, including ones unintended or even unimagined by you. The rest is documentation and error reporting. Just make any current limits transparent to users and maybe push them (the limits) further in a future release, if you think it makes sense. Of course, doing it right (whatever that means) in the first shot would also be nice.

Copy link
Member

Choose a reason for hiding this comment

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

I was mainly seeing these as tools for reporting, not doing stuff that affects the execution of the spec/feature.

Developers are creative :-).

Irregardless of the intentions, even in the reporting, there is a chance to end up with an unexpected exception. My first thought was to just to retrow it. The (filtered) stacktrace is not perfect, but in my opinion is readable enough - the extension failed in the "blockExited" method:

java.lang.RuntimeException: Unexpected during reporting

	at org.spockframework.boot2.EnforceJpaSessionFlushExtension$1$1.blockExited(EnforceJpaSessionFlushExtension.java:72)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.spockframework.boot2.EnforceJpaSessionFlushSpec.foo - parameterized(EnforceJpaSessionFlushSpec.groovy:50)

In general, I would say the accidental errors should rather be re-thrown.

Looking from my (possibly) extension point of view, the exception thrown in the listener should be considered the same way as exceptions thrown in regular statements in (for example) the "when" block - to control flow or anything else (e.g. "constraint violation" exception from the flush() in the blockExited for the when block, could be handled in by the thrown() section - as a regular "constraint violation"). I don't know, if it complicates the implementation.

Copy link
Member

@szpak szpak Mar 15, 2023

Choose a reason for hiding this comment

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

Btw, you were right Leonard, the aforementioned yesterday construction doesn't work well with the parameterized feature. interceptIterationExecution() adds listener for every iteration and I end up with n listeners for n iterations which is not always desirable :). The listener cannot be created just for the first one, as the invocation in the second iteration would refer to the invocation from the first one. It should be possible to keep the reference of added listeners and remove former in the later iteration, but it's very inelegant and fragile :-/.

Update. AtomicBoolean per iteration/listener which allows for just a single listener execution (here for WHEN) works without the removal, but still, it is a workaround.

Update 2. Even better:

if (invocation.getIteration().getIterationIndex() != iterationInfo.getIterationIndex()) {
    return;
}

but still, just a workaround.

@kriegaex
Copy link
Contributor

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
Copy link
Member Author

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

try {
org.spockframework.runtime.SpockRuntime.callEnterBlock(this.getSpecificationContext(), new org.spockframework.runtime.model.BlockInfo(org.spockframework.runtime.model.BlockKind.WHEN, []))
foobar = this.foobar()
org.spockframework.runtime.SpockRuntime.callExitBlock(this.getSpecificationContext(), new org.spockframework.runtime.model.BlockInfo(org.spockframework.runtime.model.BlockKind.WHEN, []))
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

import org.spockframework.runtime.model.IterationInfo;

public interface IBlockListener {
void blockEntered(IterationInfo iterationInfo, BlockInfo blockInfo);
Copy link
Member

Choose a reason for hiding this comment

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

Good to know, I haven't checked the data-driven feature yet. Nevertheless, my use case with calling JPA flush after the when block perfectly matches BlockListener, so maybe you will figure our a way to make it work also with data-driven features :).

@leonard84 leonard84 force-pushed the add-block-listener-support branch from c3f30eb to 0f3197e Compare May 7, 2023 05:41
@szpak
Copy link
Member

szpak commented Oct 2, 2023

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
Copy link
Member Author

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

@leonard84
Copy link
Member Author

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.

context.setCurrentBlock(blockInfo);
List<IBlockListener> blockListeners = currentIteration.getFeature().getBlockListeners();
if (blockListeners.isEmpty()) return;
blockListeners.forEach(blockListener -> blockListener.blockEntered(currentIteration, blockInfo));
Copy link
Member

Choose a reason for hiding this comment

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

How about extract method notifyListeners:

void notifyListeners(IterationInfo currentIteration, Consumer<IBlockListener> code){
  List<IBlockListener> blockListeners = currentIteration.getFeature().getBlockListeners();
  if (blockListeners.isEmpty()) return;
  blockListeners.forEach(code);
}

@leonard84
Copy link
Member Author

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

Copy link
Member

@szpak szpak left a comment

Choose a reason for hiding this comment

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

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?

IterationInfo currentIteration = context.getCurrentIteration();
List<IBlockListener> blockListeners = currentIteration.getFeature().getBlockListeners();
if (blockListeners.isEmpty()) return;
blockListeners.forEach(blockListener -> blockListener.blockExited(currentIteration, blockInfo));
Copy link
Member

Choose a reason for hiding this comment

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

Is this ...isEmpty()) return; a kind of an "optimization" or we don't want to call forEach() for any other reason?

    default void forEach(Consumer<? super T> action) {
        Objects.requireNonNull(action);
        for (T t : this) {
            action.accept(t);
        }
    }

Copy link
Member

@Vampire Vampire left a comment

Choose a reason for hiding this comment

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

Some things missing in multiple places:

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

@@ -42,6 +43,8 @@ public class FeatureInfo extends SpecElementInfo<SpecInfo, AnnotatedElement> imp
private final List<IMethodInterceptor> initializerInterceptors = new ArrayList<>();
private final Map<MethodInfo, List<IMethodInterceptor>> scopedMethodInterceptors = new HashMap<>();

private final List<IBlockListener> blockListeners = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Should we only have them on FeatureInfo, or also on other levels like IterationInfo and SpecificationInfo or maybe even some global spot?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think having them just here is enough. Having them on SpecificationInfo is convenience over calling allFeatures*.addBlockListener, or do you think we'd save a significant amount of memory by moving them up.

As for on IterationInfo I fail to see a use-case where you'd only be interested in one iteration for reporting. As noted in the JavaDoc performing side effects is discuraged, so I don't see that as an argument in support.

Copy link
Member

Vampire commented Mar 21, 2024

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

@leonard84
Copy link
Member Author

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
Copy link
Member

Vampire commented Mar 22, 2024

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
Copy link
Member Author

Forgot to publish my responses 😅

@leonard84
Copy link
Member Author

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
Copy link
Member

szpak commented Apr 25, 2024

@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
Copy link
Member Author

@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
Copy link
Member

szpak commented May 18, 2024

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?

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.
Prior to this commit, IRunListener.error(ErrorInfo) didn't give any
context where the error happened.
@leonard84
Copy link
Member Author

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
Copy link
Member

szpak commented May 22, 2024

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
Copy link
Member Author

@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
Copy link
Member Author

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

@leonard84
Copy link
Member Author

@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
Copy link
Member

szpak commented May 31, 2024

@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
Copy link
Member Author

@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
Copy link
Member

szpak commented May 31, 2024

@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:
szpak/spock-jpa-flush-enforcer@54d00a3

@szpak
Copy link
Member

szpak commented Jun 3, 2024

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
Copy link
Member Author

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
Copy link
Member

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?

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.

Allow access to the currently executing block object
5 participants