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.

Sign up for GitHub

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

Jump to bottom

Do not by default delay the execution of test cases #1170

Closed
wants to merge 2 commits into from
Closed

Do not by default delay the execution of test cases #1170

wants to merge 2 commits into from

Conversation

brasmusson
Copy link
Contributor

@brasmusson brasmusson commented Aug 7, 2017 •
edited
Loading

Summary

Do not by default delay the execution of test cases until all have been processed through the filter chain.

Details

  • Do not by default delay the execution of test cases until all have been processed through the filter chain.
  • Change the name of the TestRunStarted event to TestCount event.
  • Add the option --count-first which ensures that the TestCount event is issued before any test case is executed.

Depends on cucumber/cucumber-ruby-core#147

Motivation and Context

See #1082 (comment).
Fixes #1167.

How Has This Been Tested?

The automatic test suite has been updated to verify this behaviour.

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.

@ghost ghost assigned brasmusson Aug 7, 2017
mattwynne
mattwynne reviewed Aug 17, 2017
View reviewed changes
features/docs/events/test_count_event.feature Outdated
"""
And a file named "features/support/events.rb" with:
"""
AfterConfiguration do |config|
config.on_event :test_run_started do |event|
config.out_stream.puts "test run started"
config.on_event :test_count do |event|
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd like this to be called test_case_count

WDYT?

@mattwynne
Copy link
Member

Thanks for pushing away at this @brasmusson, I'll be glad to have the code back to normally not processing all the test cases before execution starts.

Two big thoughts here:

  1. Rather than having a configuration switch, could the BroadcastTestCountEvent filter interrogate the config to see if there are any listeners subscribed to the test_count event, and act as a no-op if there aren't any?

  2. Wouldn't it be more useful to have an event that surfaces a complete list of all the test cases, rather than just a count?

What do you think?

@brasmusson brasmusson added this to the 3.0 milestone Sep 8, 2017
@brasmusson
Copy link
Contributor Author

There are a couple of choices here:

  • Optional events?
    Should a "test count" event be optional, and only issued when batching up the test cases before execution? So far we do not have any optional events. On the other hand issuing the "test count" event after the test case has executed, as currently in the PR, does not seem very useful.
  • The possibility to query the event bus for handlers, or the existence of handlers, for events?
    So far the event sources are ignorant of the existence of, or the identity of any handlers for the events they issue. Since handlers can register themselves after events have started to be issued, if an event should only be issued if there are any handlers registered, then we have to consider when the latest time to register that handler and still trigger the issuing of the optional event. Should that time be at the time of the afterConfiguration-hook, or immediately before the issuing of the optional event?

In this RP I choose not to introduce any new concepts (optional events of querying the event bus for handlers), therefore a configuration switch is used, and the result is a change in the execution order and a reordering of events.

@mattwynne mattwynne modified the milestones: 3.0, 3.x Sep 27, 2017
@brasmusson
Do not by default delay the execution of test cases
50b73aa 
* Do not by default delay the execution of test cases until all have been
  processed through the filter chain.
* Change the name of the TestRunStarted event to TestCount event.
* Add the option --count-first which ensures that the TestCount event is
  issued before any test case is executed.
@brasmusson brasmusson mentioned this pull request Nov 5, 2017
Closed
6 tasks
@brasmusson
Make the test case count event conditional.
f630f61 
The test case cound event is issued only if a handler for it has been
registered. Only in that case are the test case execution delayed until
all test cases have been filtered.
@brasmusson brasmusson force-pushed the test-case-count-event branch from 808a5f1 to f630f61 Compare November 5, 2017 14:31
@brasmusson
Copy link
Contributor Author

@mattwynne I've update the PR so that the test_case_count event is only fired if a listener has registered itself or the event.

@mattwynne
Copy link
Member

Nice one @brasmusson!

I was thinking it would be more generally useful if it send a list of all the test case file:line locations. You could count that, but you could also use it to look up (and render details about) the forthcoming test cases' scenarios from Gherkin documents.

Do you think it makes sense to replace this event with that one now, or as part of another iteration?

@stale
Copy link

stale bot commented Jan 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 7, 2018
@brasmusson brasmusson added the 🧷 pinned Tells Stalebot not to close this issue label Jan 7, 2018
@stale
Copy link

stale bot commented Jan 14, 2018

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this Jan 14, 2018
@brasmusson brasmusson removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 14, 2018
@lock
Copy link

lock bot commented Jan 14, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2019
@mattwynne mattwynne deleted the test-case-count-event branch May 1, 2021 05:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattwynne mattwynne mattwynne left review comments

Assignees

@brasmusson brasmusson

Labels
🧷 pinned Tells Stalebot not to close this issue
Projects
None yet
Milestone
3.x
Development

Successfully merging this pull request may close these issues.

Make the TC count event optional
4 participants
@brasmusson @mattwynne @aslakhellesoy @aurelien-reeves