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

[Core] Add UnusedStepsSummaryPrinter #1648

Merged
merged 7 commits into from Jun 4, 2019

Conversation

@timtebeek
Copy link
Contributor

commented Jun 3, 2019

Summary

Adds a plugin to find and report on unused steps.

Details

As discussed in #1634 it uses the new StepDefinedEvent to locate all registered steps, and prints a summary of unused steps to the argument file.

Motivation and Context

Allows to easily find unused steps, which might be indicative of removed functionality or missed test coverage.

How Has This Been Tested?

Not yet; as @mpkorstanje indicated some hesitation on whether this would warrant adoption. This PR is to record that discussion, and will be updated depending on the outcome. Same goes for the current Java8 baseline, so it might need to target a future version rather than the next.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Could you show me an example of the output?

@timtebeek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

The current output is:

2 Unused steps:
SampleSteps.java:17: this step is unused
SampleSteps.maar(): another unused step

For a sample project here: https://github.com/timtebeek/cucumber-jvm-issues-1633/tree/step-defined-event/src/test/java/com/github/timtebeek

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Looks good. I was initially thinking we should use the same format as the default summary printer, but we may as well add it to the default summary printer. That way there is no need for an additional --dry-run or separate configuration (for most use cases).

So let's add it to the default summary printer!

For example:

Undefined scenarios:
io/cucumber/skeleton/belly.feature:3 # a few cukes
io/cucumber/skeleton/belly2.feature:3 # a few cukes

Unused steps:
SampleSteps.java:17 # this step is unused
SampleSteps.maar() # another unused step

2 Scenarios (2 undefined)
6 Steps (4 undefined, 2 passed)
2 Unused steps 
0m0.138s
  • The separator is a hash rather then a colon.
  • If there are no unused steps those sections need not be printed.
  • The locations should be printed in yellow unless monochrome is enabled.

image

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Okay. That was a terrible idea. Its nice when running the full suite but a nuisance when running a selection.

Let's not add it, but let's use the same format.

@timtebeek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Running a selection is indeed why I had implemented this as a separate plugin so far; thanks for the suggestions; I've updated the plugin accordingly. Not really sure how to cover this with tests, and/or if we're still on track for an eventual merge..

@coveralls

This comment has been minimized.

Copy link

commented Jun 4, 2019

Coverage Status

Coverage increased (+0.004%) to 85.954% when pulling 786ba1e on add-UnusedStepsSummaryPrinter into 73bbe3d on master.

@timtebeek timtebeek removed the Java 8 label Jun 4, 2019

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

We are good. I think a simple unit test where events go in and and the right output string is produced should be sufficient.

@timtebeek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Perfect; added UnusedStepsSummaryPrinterTest in 679dd3f , hope that matches what you were looking for. Anything else before we can consider a merge?

@timtebeek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Thanks for directly applying your own suggestions.. as a note: I'd used package private modifier as the inner class event handlers otherwise need to use syntactic accessors to access the private fields (or so my IDE tells me), but the code should work fine as it is now, and probably clearer.

Shall I merge? Or feel free to merge of that's faster.. :) great to see this added, saves me copy and pasting it where I need it.

@mpkorstanje mpkorstanje changed the title Create UnusedStepsSummaryPrinter [Core] Create UnusedStepsSummaryPrinter Jun 4, 2019

@mpkorstanje mpkorstanje changed the title [Core] Create UnusedStepsSummaryPrinter [Core] Add UnusedStepsSummaryPrinter Jun 4, 2019

@mpkorstanje mpkorstanje merged commit 311c6f8 into master Jun 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpkorstanje mpkorstanje deleted the add-UnusedStepsSummaryPrinter branch Jun 4, 2019

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

It's a new github feature. Really nice.

I've merged it. I've been using a format that makes it look reasonably nice. But I still need to document it so other people can use it too.

@timtebeek

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Perfect, thanks for the review/guidance/merge! :)

@mpkorstanje

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

You're welcome. Anytime.

martinsachs added a commit to eHealthExperts/cucumber-jvm that referenced this pull request Aug 9, 2019
[Core] Add UnusedStepsSummaryPrinter (cucumber#1648)
## Summary

Adds a plugin to find and report on unused steps.

## Details

As discussed in cucumber#1634 it uses the new StepDefinedEvent to locate all registered steps, and prints a summary of unused steps to the argument file.

## Motivation and Context

Allows to easily find unused steps, which might be indicative of removed functionality or missed test coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.