renatoathaydes / spock-reports

This project creates a global extension to Spock to create test reports.
Apache License 2.0
273 stars 68 forks source link

Where code blocks are missing their source code #237

Closed Gleethos closed 1 year ago

Gleethos commented 1 year ago

This is a problem because certain data table entries will become unreadable when turned into reports because toString() often distorts their representation.

Consider the following example:


    def 'My API will throw exceptions if used incorrectly.'(
            Class<Exception> type, Consumer<MyThing > errorCode
    ) {
        given :
        MyThing t  = new MyThing()

        when :
        errorCode(t)

        then :
        var exception = thrown(type)
        and :
        exception.message != "" && exception.message.length() > 13

        where :
        type                          | errorCode
        IllegalArgumentException      | { MyThing x -> x.onlyPositive(-1)                }
        NullPointerException          | { MyThing x -> x.dontLikeNull(null)              }
        UnsupportedOperationException | { MyThing x -> x.workInProgress("do something")  }
        Exception                     | { MyThing x -> x.iDontWork_ever()                }
    }

So when I try to make this into living documentation using the data table entries using the dataValues in my template file:

features.eachFeature { name, result, blocks, iterations, params ->
    params.each { p -> /* build table header */ }
    iterations.each { iteration -> iteration.dataValues /*do data row*/ }
}

I get something like this:

exception errorCode
IllegalArgumentException This_Spec$__spock_feature_0_1prov1_closure1@7f9aae2e
NullPointerException This_Spec$__spock_feature_0_1prov1_closure2@780c3492
UnsupportedOperationException This_Spec$__spock_feature_0_1prov1_closure3@7cf0a48b
Exception This_Spec$__spock_feature_0_1prov1_closure4@7c36cc9b

However, what I really want is this:

exception errorCode
IllegalArgumentException { MyThing x -> x.onlyPositive(-1) }
NullPointerException { MyThing x -> x.dontLikeNull(null) }
UnsupportedOperationException { MyThing x -> x.workInProgress("do something") }
Exception { MyThing x -> x.iDontWork_ever() }

I understand that this is not what the dataValues is for, what I would expect is that there is the actual code of the where block in the block.sourceCode field, but it is always missing! After looking into the code I noticed that there is a simple condition preventing the where block code from being read, I removed it and it worked perfectly well.

I would be happy to fix this if that's okay, this would not change anything except it would open up more option for users. This is particularly interesting to me since we are using this plugin to generate test suite browsers for pure documentation purposes so that newcomers can learn how to use our libraries and APIs without having to dig through the source code files...

renatoathaydes commented 1 year ago

Hi @Gleethos . I don't believe this to be a problem with Spock reports itself.

Where blocks are represented as tables of data in the reports, not source code, and I would like to keep it that way. In my opinion, which you are free to ignore, it's not nice to use lambdas as examples in tests. Examples are meant to be pure data. That ensures tests stay simple, and reports are clear.

You may ask how could you avoid using lambdas as examples in your tests, which is a valid question.

I have the following strategy, normally, and you can probably come up with others...

def 'my tests'( Function<MyThing, Object> example ) {
    given:
    def myThing = new MyThing()

    when: 'something'
    example( myThing )

    then: 'error'
    thrown expectedException

    where:
    example               || expectedException
    Examples.OnlyPositive || IllegalArgumentException
    Examples.NoNulls      || NullPointerException
}

enum Examples implements Function<MyThing, Object> {
    OnlyPositive, NoNulls; // more

    @Override
    Object apply( MyThing myThing ) {
        switch ( this ) {
            case OnlyPositive:
                return myThing.onlyPositive( -1 )
            case NoNulls:
                return myThing.dontLikeNull( null )
            default:
                throw new IllegalStateException( "Case not covered: $this" )
        }
    }
}
renatoathaydes commented 1 year ago

A more generic (and more dynamic) and less verbose way to do this:

Create an example class to be used when functions are to be tested:

@TupleConstructor
class ExampleFunction {
    String name
    Function fun

    def apply( arg ) {
        fun( arg )
    }

    @Override
    String toString() { name }
}

Use that as examples:

        where:
        example                                                    || expectedException
        new ExampleFunction( 'only positive', { it.onlyPositive( -1 ) } )   || IllegalArgumentException
        new ExampleFunction( 'no nulls',       { it.dontLikeNull( null ) } ) || NullPointerException
Gleethos commented 1 year ago

Hi @renatoathaydes, thank you for your quick response! Also, because I believe I have not yet acknowledged it: Thank you for creating and maintaining this powerful report engine! I am fully convinced that this project is among the most important contributions to the evolution of software quality in the JVM ecosystem, and you have my utter most respect for maintaining this wonderful piece of software. I truly mean that!

Where blocks are represented as tables of data in the reports, not source code, and I would like to keep it that way.

I believe there is a misunderstanding here as to what I am suggesting! This is not a proposal to change the way where blocks are currently generated in existing report generation templates, and I am also not suggesting that the API exposed to the templates should change in any breaking way. What I am requesting is simply to allow for access of the where source code in custom template files like for example the ones we are using to generate documentation. I am also not suggesting to parse the where block source code in any way, but merely expose it without any distortion. So for example the block:

where : 'We use the following test data.'
day        | age
Day.MONDAY |  24
Day.FRIDAY |  42

should also be somehow available as a list of lines, exactly like all the other code blocks:

['day        | age', 'Day.MONDAY |  24', 'Day.FRIDAY |  42'] 

I mostly agree with your objection of the usage of closure / lambda expressions in data tables, however this was merely meant to serve as a simple example to quickly demonstrate how certain data table entries are distorted in reports which compromises their ability to serve as API documentation of our code, which is what we are working on... Here some more examples:

Enums

The string representation of an enum might merely be its name, however it does not show the name of the enum itself, which would be necessary for a reader of our documentation in order to grasp what it is and how to use it. For example, we maintain a UI library with the enum ScrollPolicy having instances like NEVER, ALWAYS, ON_DEMAND, whose meanings are unclear without their prefix, as is written in the data table in raw code form.

Records / Data Classes

Certain record implementations or classes with a data centric purpose might not have a string representation reflecting their types and how to create them. Consider for example the record type Point with the constructor new Point(2, 6) and possible string representations of (2, 6), [2, 6], [2|6]... none of which tell a reader of the data table what kind of data this is and most importantly how they can create and use it themselves.

Instantiation

This ties into the previous point of data centric types which may themselves have a unique way of instantiating them like for example factory methods: Plane.ofXYAt(3, -4), Sphere.at(-3, 6), Label.saying("hello") ...or fluent builder APIs... Nda.of(String).shape(2, 4, 3).andFill('a'..'z')

So for example I have tests with data tables very similar to this:

where : 'We can assign the following expression to the "array" variable:'
array << [
        Nda.ofBytes().withShape(2, 5).andFill(-4..7),
        Nda.ofInts().vector(42..73)
        Nda.ofFloats().eye(7) // 7x7 identity matrix
   ]

Constants

This is very similar to the problem with enums. Constants are an important part of many APIs and it would be nice if they are highlighted as such so that a reader of the documentation/report could then adopt their usage. A classic example is the Math API which exposes important constants like Math.PI and Math.E, A reader of our documentation should be confronted with their declaration and not their values, because these values are often times not recognizable as anything a reader can relate, whereas their declarations are! The numbers 3.1415. and 2.718.. are famous numbers which are probably recognized by most people... However there are numerous fields with very special constants. Would you for example identify the number 299792458 as the speed of light in m/s? When trying to use this plugin to generate usage documentation it is confusing when a reader sees 299792458 instead of simply PhysicalConstants.LIGHT_SPEED.

Data Types

One last problem that comes to mind is that even for the most basic types their type information and format is currently lost.

This is extremely important when building reports intended to be not just reports describing the success and failure of our APIs but how a user can use the API themselves for their use-cases!


Again, I am in no way suggesting the reform how this plugin currently generates its reports or if they should contain where code or not. I understand that this is a popular project with many many users, so breaking its API would be bad. This issue is merely a request for the inclusion of the where source code in the API exposed to the template files in some form or another, so that we can account for the above mentioned "issues" in our template files.

If all of this does not seem important to you than that is fine, I respect the great work you have done with this project, but I hope that you might see this as a little enhancement so that the power users of spock-reports like myself can get the most out of it. I hope all of this makes sense.

Thank you again for this wonderful project! I really appreciate what you do.

renatoathaydes commented 1 year ago

I truly mean that!

Thanks man. That really makes me happy :)

I believe there is a misunderstanding here as to what I am suggesting!

I see now that I'd read your question and failed to understand the full request! Sorry about that.

I will go through your suggestions and get back to you in the next couple of days. But I believe we can make this happen.

Let's get your other issue fixed first, then I can address this.

Also, please notice there's a Groovy 4 branch now, groovy4. Do you plan to use that?

renatoathaydes commented 1 year ago

Hi again @Gleethos,

After looking into the code I noticed that there is a simple condition preventing the where block code from being read, I removed it and it worked perfectly well.

Ok, so if I remove that check (and remove the code from where blocks from the default HTML report given that it will always look "weird", like x | y) that's enough for you to do what you want to do? I think I might still be missing something because this is what the HTML report looks like if I allow the where block to display the source:

Screen Shot 2022-10-31 at 19 30 19

If that's going to work anyway for you, I will release this fix and #235 later today.

renatoathaydes commented 1 year ago

The default template report (markdown) also looks weird:

Screen Shot 2022-10-31 at 19 40 56
Gleethos commented 1 year ago

Thank you for considering adding this! Sorry for my delayed response, I have been busy the past few days.

If that's going to work anyway for you, I will release this fix and #235 later today.

Almost, except that all of the code lines should be exposed to the template file. From the screenshots you posted it looks like only the first where code block line is displayed. So I would expect there to be a list of all line strings for the where block just like all other blocks.

So for example for the where block...

where : 'We use the following names:'
name << [
        'Adam',
        'Zoey',
        'Tom'
    ]

I would expect the following data exposed to the template:

[
    "name << [",
    "        'Adam',",
    "        'Zoey',",
    "        'Tom'",
    "    ]"
]

That way we can parse the data table ourselves in a way which is more suitable as documentation.

Gleethos commented 1 year ago

Also, please notice there's a Groovy 4 branch now, groovy4. Do you plan to use that?

I am not sure what you mean. Do you mean if I plan to use Groovy 4 for my projects test suite? Or do you mean if I plan using it in a pull request?

I am certainly planing to migrate to Groovy 4.

renatoathaydes commented 1 year ago

I am certainly planing to migrate to Groovy 4.

That was my question... was hoping some people could test the Groovy 4 branch before I release that.

Gleethos commented 1 year ago

Sure! I will run it on a few hundred unit tests, and inform you of any problems.

Gleethos commented 1 year ago

I tested the Groovy 4 branch on all my tests and it worked well! The indent issue #236 seems fully fixed, the indents are all preserved, which is great. I also checked out your changes and looked at them in more detail. This is much cleaner than my hacky approach! Thank you!

Also, I locally disabled the condition...

if ( statement.statementLabel == 'where' ) {
    waitForNextBlock = true
}

at line 183 in the VividAstInspector and actually got exactly what I expected! This seems to fix this issue #237! However this does break tests it seems because the existing report generators seem to notice that the where block is no longer empty and then change their output like in the screenshot you posted...

Would it be possible to remove the above condition and then simply adjust the existing report generators to explicitly ignore where block code?

Ok, so if I remove that check (and remove the code from where blocks from the default HTML report given that it will always look "weird", like x | y) that's enough for you to do what you want to do?

If that's going to work anyway for you, I will release this fix and https://github.com/renatoathaydes/spock-reports/issues/235 later today.

If the check you were mentioning before is in fact the one I was just referencing I am happy with this solution and would consider this fully fixed yes! :)

Edit: Should I open a PR for this?

renatoathaydes commented 1 year ago

@Gleethos if you open a PR that is definitely doing what you think #237 requires, I am more than happy to accept that... I did remove that check, and then changed the default HTML report to filter blocks to exclude the where blocks (in effect, just changing where the check was happening), but I didn't think that was providing the examples in the where block. But if you see that happening, pls make a PR, and I believe the tests will pass once you do this:

Screen Shot 2022-11-03 at 18 52 18

And the same probably in the template report creator:

Screen Shot 2022-11-03 at 18 54 43
Gleethos commented 1 year ago

I tested my changes on some new report template code and just now pushed another missing fix preventing that sometimes the first where code line is not included... I reviewed countless of report files generated based on my changes and am confident that this is now fully fixed.