Skip to content

Add support for repeatable annotations for local extensions (#1030)#1118

Merged
leonard84 merged 4 commits intospockframework:masterfrom
Vampire:repeatable-annotations
Jul 9, 2020
Merged

Add support for repeatable annotations for local extensions (#1030)#1118
leonard84 merged 4 commits intospockframework:masterfrom
Vampire:repeatable-annotations

Conversation

@Vampire
Copy link
Copy Markdown
Member

@Vampire Vampire commented Mar 11, 2020

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 11, 2020

Codecov Report

Merging #1118 into master will increase coverage by 0.18%.
The diff coverage is 91.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1118      +/-   ##
============================================
+ Coverage     75.74%   75.93%   +0.18%     
- Complexity     3619     3641      +22     
============================================
  Files           392      392              
  Lines         10997    11089      +92     
  Branches       1348     1369      +21     
============================================
+ Hits           8330     8420      +90     
+ Misses         2192     2190       -2     
- Partials        475      479       +4     
Impacted Files Coverage Δ Complexity Δ
...ava/org/spockframework/compiler/SpecAnnotator.java 94.39% <88.46%> (-5.61%) 34.00 <15.00> (+17.00) ⬇️
...java/org/spockframework/compiler/AstNodeCache.java 100.00% <100.00%> (ø) 1.00 <0.00> (ø)
...va/org/spockframework/runtime/ExtensionRunner.java 94.80% <100.00%> (+1.36%) 25.00 <11.00> (+5.00)
...ava/org/spockframework/runtime/model/NodeInfo.java 100.00% <100.00%> (ø) 15.00 <1.00> (+1.00)
.../spockframework/mock/runtime/MockInstantiator.java 63.63% <0.00%> (-3.04%) 5.00% <0.00%> (ø%)
.../java/org/spockframework/util/DataVariableMap.java 81.63% <0.00%> (-1.71%) 14.00% <0.00%> (ø%)
...org/spockframework/compiler/ConditionRewriter.java 56.06% <0.00%> (-0.41%) 44.00% <0.00%> (ø%)
...ork/runtime/extension/builtin/UnrollExtension.java 100.00% <0.00%> (ø) 23.00% <0.00%> (ø%)
...ck/runtime/DynamicProxyMockInterceptorAdapter.java 100.00% <0.00%> (ø) 4.00% <0.00%> (ø%)
...rg/spockframework/compiler/WhereBlockRewriter.java 91.09% <0.00%> (+0.05%) 86.00% <0.00%> (ø%)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36a2430...74a4a97. Read the comment docs.

@Vampire
Copy link
Copy Markdown
Member Author

Vampire commented Mar 12, 2020

Here the actual repeatable annotations can be looked at if wanted: Vampire/spock@repeatable-annotations...repeatable-annotations-combined#files_bucket
They will come as own PRs if this one is through.

@Vampire Vampire force-pushed the repeatable-annotations branch 2 times, most recently from 3beaaad to 969093f Compare March 31, 2020 15:03
@Vampire Vampire force-pushed the repeatable-annotations branch 2 times, most recently from 602810d to 86d9a5d Compare April 5, 2020 23:37
@Vampire Vampire force-pushed the repeatable-annotations branch 4 times, most recently from cfaf735 to 07bddf5 Compare April 20, 2020 22:57
@Vampire Vampire force-pushed the repeatable-annotations branch from 07bddf5 to 78b09fb Compare April 22, 2020 17:40
@Vampire Vampire force-pushed the repeatable-annotations branch 5 times, most recently from 40f515c to d3dc91c Compare May 7, 2020 22:42
@Vampire Vampire force-pushed the repeatable-annotations branch from d3dc91c to 3cc468a Compare May 19, 2020 10:17
@Vampire Vampire force-pushed the repeatable-annotations branch from 3cc468a to 15e87f8 Compare May 31, 2020 01:50
def "should work properly on feature"() {
expect:
runner.runSpec """
${buildContainer(contained, containerWithList)}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If find these tests really hard to read, since you basically have to execute the template code in your head to get an idea of what the actual test looks like.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

How do you suggest that I improve it?
Being very verbose and long in the where block having the annotations there manually written and hard to read properly?
Adding some comments to what the test does?
I tried to write easy to understand and read and at the same time concise tests.
The where table specifies how many individual annotations, how many contained in the container annotation manually and if one whether directly or inside a list to use and how many extension calls are expected.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would think about generating the tests based on the code you already have and commit both. I think this where DAMP is more important than DRY explanation

What are your thoughts @szpak @masooh ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think committing both makes too much sense, because the next person will change the generated code not even being aware of the source, the next person after that might again be aware of the source, change the source and re-generate, overwriting the first other persons changes and so on.

I though the numbers actually are DAMP.
For example

individual | contained | containerWithList || expected
1          | 2         | true              || 3

means 1 individual annotation without container, two annotations in the container and the container value as list should result in 3 times the extension executed.
I actually inteded this to be actually good readable. :-(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For me the test is very hard to read and it took me a while to find out what happens.

Nevertheless I must admit that I don't have a better solution for this scenario, I have to think about it.

Javadoc for buildContainer (some samples what could be generated) and for RepeatableLocalExtensionsSpec (that the test uses a dummy extension to check the behaviour) would help for sure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a bit of JavaDoc to hopefully make the tests clearer and easier to interpret :-)

@Vampire Vampire force-pushed the repeatable-annotations branch from 6c3b86f to 15007c8 Compare June 29, 2020 17:08
@Vampire
Copy link
Copy Markdown
Member Author

Vampire commented Jul 1, 2020

What I mean is using

Yes, I was fully aware of what you meant.
And I even tried as I was not fully sure whether it will work or not.
But it will not work due to the reasons I described in my answer.
You will not get this working with the tests currently committed.

@Vampire Vampire force-pushed the repeatable-annotations branch from 6c3b86f to 9808cd1 Compare July 4, 2020 23:58
@Vampire Vampire requested review from leonard84 and masooh July 4, 2020 23:59
@leonard84 leonard84 merged commit 6ddae64 into spockframework:master Jul 9, 2020
@Vampire Vampire deleted the repeatable-annotations branch July 9, 2020 12:34
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.

3 participants