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

Allow adding tags dynamically. #213

Merged
merged 3 commits into from Jul 7, 2016
Merged

Conversation

Airblader
Copy link
Contributor

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

}
}

private void addTags( List<Tag> tags ) {
this.reportModel.addTags( tags );
this.scenarioModel.addTags( tags );
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

@Airblader
Copy link
Contributor Author

Rebased to the current master.

@janschaefer
Copy link
Contributor

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?

@Airblader
Copy link
Contributor Author

Hm, I'm not sure. CurrentStep allows access to the current step method, but tags are related to entire scenarios, not individual steps. So semantically it doesn't seem to belong into CurrentStep to me. Is there any situation where CurrentStep can be used, but getScenario() can't?

An alternative would be to expose CurrentStep#getScenario() which would then also allow adding tags from there. At the same time I feel that it proves my point about getting to scenario-wide information from step-specific mechanisms… :-)

@janschaefer
Copy link
Contributor

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.

@Airblader
Copy link
Contributor Author

@janschaefer OK, sure. Should we remove ScenarioBase#addTag again, then?

@Airblader
Copy link
Contributor Author

Small addendum:

that is returned by CurrentStep#getScenario()?

Is that necessary? I still think getting to the scenario from a step sounds a little off. If we have a CurrentScenario interface, the user can just inject this directly anyway. No need to go through CurrentStep, right?

@janschaefer
Copy link
Contributor

Also true and makes sense

@janschaefer
Copy link
Contributor

I am still not quite sure about the use cases of this feature. :-)

@Airblader
Copy link
Contributor Author

Also true and makes sense

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 section method in ScenarioBase.

@Airblader
Copy link
Contributor Author

Airblader commented Jul 3, 2016

I am still not quite sure about the use cases of this feature. :-)

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.

@Airblader
Copy link
Contributor Author

Airblader commented Jul 3, 2016

Programmatic tags could also be useful if the behavior of a stage method depends on the scenario state, for example in a When method like this:

public SELF the_order_is_submitted() {
    currentScenario.addTag( Order.class, paymentMethod );
    // …
}

Or a Given method like this which illustrates using the provided argument:

public SELF the_backend_responds_with( @FormatErrorCode BackendErrorCode error ) {
    currentScenario.addTag( BackendFailure.class, error.toReadableFormat() );
    // …
}

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.

@Airblader
Copy link
Contributor Author

Airblader commented Jul 3, 2016

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.

@Airblader
Copy link
Contributor Author

Airblader commented Jul 5, 2016

So just to summarize on what we discussed today:

  • We want to allow tag annotations on stage methods and classes.
  • We want to use a CurrentScenario interface similar to CurrentStep.
  • [] We want to move section() to said interface.
  • We want to allow dynamic tags through CurrentScenario#addTag.

@Airblader Airblader force-pushed the feature-172 branch 2 times, most recently from 62eb3ee to b767837 Compare July 5, 2016 19:05
@Airblader
Copy link
Contributor Author

I'm actually not sure that we should move addSection. We'd also have to remove section() from ScenarioTestBase, but I think that section() should be in there just like given() / when() / then() because all of those methods are used in a way where their order is relevant to both the output and the way the scenario reads. So I'll skip this for now.

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
@janschaefer
Copy link
Contributor

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() );
Copy link
Contributor Author

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…?

@Airblader
Copy link
Contributor Author

@janschaefer OK, good to go! See my one comment above.

@janschaefer janschaefer merged commit e162dcf into TNG:master Jul 7, 2016
@janschaefer
Copy link
Contributor

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding tags dynamically
2 participants