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

Where code blocks are missing their source code #237

Closed
Gleethos opened this issue Oct 29, 2022 · 13 comments
Closed

Where code blocks are missing their source code #237

Gleethos opened this issue Oct 29, 2022 · 13 comments

Comments

@Gleethos
Copy link

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

renatoathaydes commented Oct 30, 2022

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

  • Create an enum which abstracts away the logic to be tested.
  • Most of the time, it can implement a 'native' interface like java.util.Function or some custom interface to make it monophormic.
  • In your example case, something like this:
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
Copy link
Owner

renatoathaydes commented Oct 30, 2022

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

Gleethos commented Oct 30, 2022

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.

  • 1_000L -> 1000 // int, long, byte?
  • 50 as byte -> 50 // int, long, byte?
  • 'a' as char -> a // string or letter?!?
  • 42_666.73f -> 42666.73 // double or float?
  • new byte[]{1, 2, 3} -> [1, 2, 3] // list or array? bytes, ints, longs...?

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

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

renatoathaydes commented Oct 31, 2022

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

The default template report (markdown) also looks weird:

Screen Shot 2022-10-31 at 19 40 56

@Gleethos
Copy link
Author

Gleethos commented Nov 2, 2022

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

Gleethos commented Nov 2, 2022

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

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

Gleethos commented Nov 2, 2022

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

@Gleethos
Copy link
Author

Gleethos commented Nov 3, 2022

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

@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 pushed a commit to Gleethos/spock-reports that referenced this issue Nov 4, 2022
Gleethos pushed a commit to Gleethos/spock-reports that referenced this issue Nov 9, 2022
@Gleethos
Copy link
Author

Gleethos commented Nov 9, 2022

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.

renatoathaydes added a commit that referenced this issue Nov 14, 2022
Fix #237, where code blocks are missing their source code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants