-
Notifications
You must be signed in to change notification settings - Fork 99
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
Allow adding tags dynamically. #213
Conversation
21f501b
to
1942cee
Compare
} | ||
} | ||
|
||
private void addTags( List<Tag> tags ) { | ||
this.reportModel.addTags( tags ); | ||
this.scenarioModel.addTags( tags ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you add the tags to the reportModel and the scenarioModel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the normal path of adding tags via annotation does the same. Which one shouldn't it be added to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be correct :-D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. :-) Then everything is ready from my side.
Rebased to the current master. |
Cool! I still wonder, however, whether it wouldn't be useful to have a method in the CurrentStep class to add tags to the scenario. What do you think? |
Hm, I'm not sure. An alternative would be to expose |
Yes tags belong to the Scenario and not the step. Maybe one could introduce a new interface CurrentScenario that is returned by CurrentStep#getScenario()? CurrentScenario could have a method addTag then. |
@janschaefer OK, sure. Should we remove |
Small addendum:
Is that necessary? I still think getting to the scenario from a step sounds a little off. If we have a |
Also true and makes sense |
I am still not quite sure about the use cases of this feature. :-) |
Is this also a "yes" to removing it from ScenarioBase again? :-) The reason I'm asking is because for consistency we should probably do the same for the |
Since you opened the ticket I assumed it had a valid usecase. But of course let's not merge anything if we can't verify that it's useful. Nobody wants bloat. :-) Do tags work if stage methods are annotated with them rather than the scenario? I feel like the "major" usecase here is that the user doesn't need to think about tags for every new test they write, but instead one could annotate the stage method to include a certain tag in all tests that call this method. That doesn't require programmatic tags, however; simply annotating the stage method would suffice for that. As for a need for actual programmatic tags, I could think of adding a tag based on an argument provided to a stage method. Although one could probably argue that it'd be worth having separate stage methods for such cases. |
Programmatic tags could also be useful if the behavior of a stage method depends on the scenario state, for example in a
Or a
If we decide that any of these is actually useful enough to include this feature, I'll also adapt the example to demonstrate the usecase better. |
I actually really like the idea of having stage methods provide tags rather than scenarios, at least for some cases. I think independent of the discussion about programmatic access, it would be useful to be able to define tags on stage method level. It ensures that future tests using a feature automatically get the tag as well; it would also make adding tags for a feature much easier. Likewise, it should be possible to tag an entire stage class with all stage methods inheriting this tag. For usecase-oriented tagging this would make things much more comfortable. |
So just to summarize on what we discussed today:
|
62eb3ee
to
b767837
Compare
I'm actually not sure that we should move |
This commit allows adding tags dynamically to a scenario at runtime. Furthermore, it implicitly fixes an issue where nested tags, description and href would not be evaluated if ignoreValue was set to true. fixes TNG#172
We introduce a new interface CurrentScenario similar to CurrentStep which can be injected into stages. The previously introduced addTag() method is moved from ScenarioBase to this new interface. relates to TNG#172
I agree :-) |
@@ -172,6 +174,9 @@ public void stepMethodInvoked( Method method, List<NamedArgument> arguments, Inv | |||
} else if( method.isAnnotationPresent( StepComment.class ) ) { | |||
stepCommentAdded( arguments ); | |||
} else { | |||
addTags( method.getAnnotations() ); | |||
addTags( method.getDeclaringClass().getAnnotations() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried passing the receiver from cglib instead of using getDeclaringClass
, but cglib subclasses the class to create the proxy which means any annotation not tagged with @Inherited
gets lost. I'm not really sure if either way would be better than the other, so maybe this is completely fine…?
@janschaefer OK, good to go! See my one comment above. |
Cool, thanks! |
This commit allows adding tags dynamically to a scenario at runtime.
Furthermore, it implicitly fixes an issue where nested tags, description
and href would not be evaluated if ignoreValue was set to true.
fixes #172