Adding block listener support

[sebi-hgdata]

Hey,
I've added a pull request that adds when/then block notification support.
I'm planning to use them internally for dynamic report generation (descriptions with variable expansion) by customizing the existing support.
Is this something that interests the community ? If yes, I can allocate some time to add implement any feedback that comes

---

This change is 

[Fuud]

Why String type parameter is String? May be it should be enum?
Why only when and then blocks? setup and expect can be interesting too.

[sebi-hgdata]

@Fuud thanks for the feedback. Will do some changes when I have time (btw will try to add an include block for including code that contains blocks)

Guys, there is little to no action overall on this cool project, is this something that is intended?

[argos83]

Wouldn't be useful to have both beforeBlock and afterBlock?

[sebi-hgdata]

Don't have a use case for afterBlock. My current use case for this is to have dinamic reports by resolving the block expression before sending it to the reports listener

[genakas]

I am interested in this functionality. I use Allure reports, before and after Block events would help to create parent step, and the steps inside the block would give more details, if needed.
Poor Spock reporting capabilities is something that stops test automation engineers from using Spock
What do we need in order to add also afterBlock and other labels like "expect:"?

[sebi-hgdata]

@genakas for expect see bbf4837 , it adds support for setup blocks

For before/after stuff just inject whatever code you need before/after block statements (see updateNotify method)

[genakas]

What is preventing this to be merged?

[leonard84]

I see several issues:

• first there are formatting changes to otherwise unrelated parts of the code.
• imho the AST code should not generate a loop but use a helper function to call the listeners, which in turn uses the @BlockMetadata
• I agree on beforeBlock and afterBlock
• if we add this, then it should work for all blocks as expected
• currently and: is just syntactic sugar and is removed, how do we deal with this? The user would expect to receive a separate notification for these blocks
• extending IRunListener will break all classes that do not extend from AbstractRunListener
• the block listener methods should have either access to IterationInfo or a new BlockInfo, that has IterationInfo as parent.

[robfletcher]

Looks like expect: is not handled either.

Fundamentally this is a good idea but the issues @leonard84 raises above are valid. We shouldn't include this in 1.1 at this stage but certainly should consider it after 1.1 final is released & above issues are addressed.

[genakas]

Meanwhile, I use a simple and ugly, yet functional, workaround.
I add an underscore (just to make it not too ugly) after the semicolon. It can use a string parameter, e.g.
when:_ "account $accountNumber is opened"
or just
and:_ //this will close the previous block
define in MyAbstractSpecification _ property and function _(String message) and have a nice report with no issues @leonard84 listed.
Other ideas:
expect: that "values are equal" or
then: check "the page is loaded"
My suggestion is to add a function call after the semicolon with AST, and implement it with an empty body in Specification. One may override the functions in his Specification subclass.

[leonard84]

Just note that _ is already used in a Specification as a wildcard matcher for mocking.

[kriegaex]

Well, but _ in Specification is static. IMO this is not a problem, or is it? Otherwise the method could still be named __, p (for "print") or whatever.

BTW, it is quite easy to avoid the abstract base spec because for people using Spock as well as Geb it would mean several base specs such as ones extending Specification, GebSpec, GebReportingSpec. Thus I solved the problem as described on StackOverflow: by using SpockConfig.groovy and declaring the printing method therein as a mixin to Specification:

import spock.lang.Specification

class LabelPrinter {
  def _(def message) {
    println message
    true
  }
}

Specification.mixin LabelPrinter

[sebi-hgdata]

@leonard84 I will have some spare time to work on this in the following 2 weeks.
Will work first resolving the obvious stuff, but we need to decide :

• on a notification approach
  • pub/sub with events emitted before and after blocks
  • the state of the event:
    • block description, type, other
• should maintain backwards compatibility
• should be extensible for other use cases not only reporting ?

[leonard84]

The greatest issue I see is with the rewrites performed by Spock for its mock handling.

class Spec extends spock.lang.Specification {

  def "simple Test"() {
    given: "a simple list"
    List subject = Mock()
    
    when: "an element is queried"
    def result = subject.get(0)
    
    then: "get is called and hello is returned"
    1 * subject.get(_) >> "hello"
    result == "hello"
  }
}

will be transformed into

@org.spockframework.runtime.model.FeatureMetadata(name = 'simple Test', ordinal = 0, line = 3, 
	blocks = [org.spockframework.runtime.model.BlockKind.SETUP['a simple list'], 
	org.spockframework.runtime.model.BlockKind.WHEN['an element is queried'], 
	org.spockframework.runtime.model.BlockKind.THEN['get is called and hello is returned']], parameterNames = [])
public void $spock_feature_0_0() {
	org.spockframework.runtime.ErrorCollector $spock_errorCollector = new org.spockframework.runtime.ErrorCollector(false)
	org.spockframework.runtime.ValueRecorder $spock_valueRecorder = new org.spockframework.runtime.ValueRecorder()
	try {
		java.util.List subject = this.MockImpl('subject', java.util.List)
		this.getSpecificationContext().getMockController().enterScope()
		this.getSpecificationContext().getMockController().addInteraction(new org.spockframework.mock.runtime.InteractionBuilder(11, 5, '1 * subject.get(_) >> "hello"').setFixedCount(1).addEqualTarget(subject).addEqualMethodName('get').setArgListKind(true).addEqualArg(_).addConstantResponse('hello').build())
		java.lang.Object result = subject.get(0)
		this.getSpecificationContext().getMockController().leaveScope()
		try {
			org.spockframework.runtime.SpockRuntime.verifyCondition($spock_errorCollector, $spock_valueRecorder.reset(), 'result == "hello"', 12, 5, null, $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(2), $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(0), result) == $spock_valueRecorder.record($spock_valueRecorder.startRecordingValue(1), 'hello')))
		} 
		catch (java.lang.Throwable throwable) {
			org.spockframework.runtime.SpockRuntime.conditionFailedWithException($spock_errorCollector, $spock_valueRecorder, 'result == "hello"', 12, 5, null, throwable)} 
		finally { 
		} 
		this.getSpecificationContext().getMockController().leaveScope()
	} 
	finally { 
		$spock_errorCollector.validateCollectedErrors()} 
}

As you can see some of the interaction logic is moved before the when code, and some is executed "before" the then code. So the blocklistener would have to call the then just before leaveScope() since that triggers the mock validation. What do we do with the mock interaction setup, there are cases where the setup code can throw exceptions, to which block should these exceptions belong to?

[kriegaex]

As usual when introducing Spock to a new team, the question of block label printing during test execution (I am not talking about test report generation here) came up. So I hope it is okay to ask if this PR is still on anyone's radar and if there is a plan how to implement block listener support (including a solution for "and").

See also #538.

[leonard84]

Yes the feature request is still on the radar, this PR will not be the basis for it though (reasons listed above). Unfortunately, we can't transform it into an issue, as the comments are still valid.

[vanjimohan]

I also would require this feature, is someone already working on the comments fixes?

[vanjimohan]

Well, but _ in Specification is static. IMO this is not a problem, or is it? Otherwise the method could still be named __, p (for "print") or whatever.

BTW, it is quite easy to avoid the abstract base spec because for people using Spock as well as Geb it would mean several base specs such as ones extending Specification, GebSpec, GebReportingSpec. Thus I solved the problem as described on StackOverflow: by using SpockConfig.groovy and declaring the printing method therein as a mixin to Specification:

import spock.lang.Specification

class LabelPrinter {
  def _(def message) {
    println message
    true
  }
}

Specification.mixin LabelPrinter

This looks interesting, is there anyway i can add screenshot images as well at block level along with these messages?

[kriegaex]

@vanjimohan, you should not quote complete messages. As for screenshots, this is kind of off-topic here as we are talking about Spock, not Geb. You are hijacking a thread.

But why don't you just try? In principle you can add any code into the _(def message) method, but then it would no longer work for all Spock tests, only for Geb tests and only if there is a valid browser page to take a screenshot of. You could also add another mixin method. I just don't see the point because IMO screenshots should be taken explicitly and not automatically for every block label. But do whatever you like and please don't answer here anymore because we are really off-topic. I just wanted to be nice and answer once because you quoted my post.

[lhupfeldt]

Any chance of this making it into 2.0 (or 1.3.x)?

[Vampire]

Pretty unlikely as 2.0 is already released and there will not be any further 1.3.x release.
Also the PR had no action for a long time and also has various conflicts with the base branch.
I guess unless someone picks up the work and somehow manages to care about Leonards comments it will not come.

[lhupfeldt]

Sad. This seems like a needed feature in order to generate proper detailed reports.

[kriegaex]

I guess unless someone picks up the work and somehow manages to care about Leonards comments it will not come.

Why would a useful and potentially popular feature like this depend on a PR? And why would a bad or suboptimal PR block a feature from being implemented from scratch? Probably, a maintainer can implement this in a better way which would not cause a PR to sit there and rot in the first place. It might not have the highest priority, if there is more important work in the backlog - no problem with that. But to say, a feature is unlikely to be implemented, just because there is no normal user capable of implementing it herself, strikes me as an odd statement. IMO, less useful stuff has been implemented and released, while this one here, bearing potential for a whole new class of extensions, remains untouched because no external contributor so far could do it right. Hmm... 🤔

[Vampire]

You either got me wrong or are twisting my words in my mouth.
He asked whether this PR will be merged.
I didn't say that not someone else could implement the feature request per-se from scratch.
And Leonard above also said the feature per-se is still on the radar but a solution will most likely not be based on this PR.

[lhupfeldt]

Well, actually I care about the feature, not the PR itself :)

[kriegaex]

I never had any intention of twisting anyone's words in their mouths, but interpreted your statement to the best of my understanding. So that kind of innuendo is not going to make anything better. Now that you expressed yourself more unambiguously, I understand better - and so do others reading this, too, I suppose. So thank you for the clarification.

[pushkar-nayama]

Can someone please look into these changes and take ahead. If it is merged then it will be good for reporting purpose.

[leonard84]

Block listener support will be implemented via #1575