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

Fix (#235) VividAstInspector not accounting for indents #236

Closed Gleethos closed 1 year ago

Gleethos commented 1 year ago

I fixed the bug described in #235, however there were 2 places where I had to adjust the specifications in the test suite because the expected results were also missing indents. I hope these changes are fine, nothing is broken and I already tested these changes on 1400+ tests, the reports were all generated well.

I also added a new test which ensures that indents are preserved.

renatoathaydes commented 1 year ago

Hi @Gleethos and thanks for your pull request.

I had a look at your changes, and even though I see you spent some effort to fix the problem, which I appreciate a lot, I would like to propose a simpler set of changes that I believe is sufficient to fix the problem described in #235.

https://github.com/renatoathaydes/spock-reports/commit/413e588f4ea128151b069ae5a942c3c6de7c76bd

Could you please check it out? If you could try running your tests with the dev branch to confirm those changes work, that would be very helpful so I can release the fix as soon as possible.

My reasons for preferring my own smaller changeset are:

Gleethos commented 1 year ago

Thank you for addressing this so quickly!

I had hoped my understanding of the code was sufficient for this change so that you wouldn't have to invest so much time. Thank you for working on this!

Sorry I have not yet had the time to check out your changes and validate them with my test suites. I will run the changes as soo as I get back to my workstation.

renatoathaydes commented 1 year ago

One thing I didn't realize was that you seem to have addressed issue #237 in this pull request. I was not looking into that as it was never mentioned in this PR! Now I understand why I had thought you made too many changes to fix only #235.

I do want your changes to fix #237 though... so can you make another PR fixing only that, as I think my fixes in dev already addressed #235 sufficiently.

Gleethos commented 1 year ago

I have not addressed #237 in my changes of this PR, although I did make an adjustment in the third commit which simply makes sure that indents are only determined on a block by block basis, meaning that the indents of one block do not change the indents of another block. So after my first two commits the following problem occurred:

    given :
def something = new Something()
    expect:
    something.isValid()

would lead to the following code strings: (If we put all code lines into one list)

[
    "def something = new Something ()",
    "    something.isValid()"
]

I did not remove the condition which forbids the inclusion of where block code in my commits! However I tried disabling them and noticed how easy is was to enable it, because this did not break anything when I tested it on my test suites...

I could have done a better job here definately, these 4 commits should have been 1 clean commit.

Gleethos commented 1 year ago

I looked into your changes and tested them locally and it seems this is fully fixed, the indents are even consistent on a block by block basis as I described in the previous comment! I will close this pull request now.

Edit:

I do want your changes to fix https://github.com/renatoathaydes/spock-reports/issues/237 though... so can you make another PR fixing only that, as I think my fixes in dev already addressed https://github.com/renatoathaydes/spock-reports/issues/235 sufficiently.

My changes did not fix #237! I commented under #237 because all that is really needed is to remove the following condition:

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

At line 183 in the VividAstInspector.

I am happy to open a PR for this!