[Core] Allow runner to register bus as concurrent or serial event source

[timtebeek]

Fixes #1515

Summary

As discussed in #1515 the pretty text output for cli/junit/testNg is also delayed for users running on a single thread from their IDE or commandline. This small change restores immediate text output when running on a single thread, only delaying when multiple threads are involved.

A better fix would be an overhaul of the PrettyFormatter, but that's considerably more work, whereas this would fix a lot of the same use cases.

Details

Do not create or use the CanonicalOrderEventPublisher for plugins when running on a single thread.

Motivation and Context

It ensures non-ConcurrentEventListener plugins get events immediately when running on a single thread, rather than delayed until all tests are run.

How Has This Been Tested?

Not yet, as it might be considered too hacky.

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.

[timtebeek]

No idea why the some JDKs suddenly get

[ERROR] Errors: 
[ERROR]   class gherkin.ast.Scenario cannot be cast to class gherkin.ast.Step (gherkin.ast.Scenario and gherkin.ast.Step are in unnamed module of loader 'app')
[ERROR]   class gherkin.ast.Background cannot be cast to class gherkin.ast.Step (gherkin.ast.Background and gherkin.ast.Step are in unnamed module of loader 'app')

but perhaps we can debug that once the rest of the code is acceptable.

[mpkorstanje]

That happens when event's are send to the pretty formatter in the wrong order.

The junit runner and test ng runner are reimplementations of the runtime. They don't use the --threads option because their frameworks provide support for parallel execution. That's probably what is going wrong.

Not sure how to detect when they are being ran in parallel.

[timtebeek]

Hmm; also can't seem to find here how to detect the number of threads JUnit4 will use; So I'm not sure this approach will work without specific support for JUnit and TestNG. I'll let it rest for a while.

[mpkorstanje]

We don't need to know the exact number of threads. Only if the execution is not guaranteed to be serial.

For JUnit Cucumber extends ParentRunner and ParentRunner has a method named setScheduler. I would assume that when a scheduler is set the execution is no longer guaranteed to be serial.

I'm at a loss for TestNG as well. But we could go with the assumption that it runs in parallel by default.

[mpkorstanje]

Pushed a thing. I'm rather unhappy about the structure of it, but it works.

It is beocmming apparent that immediately subscribing the plugins to the event bus is problematic. Would need to think about the structure.

[coveralls]

Coverage increased (+0.05%) to 85.663% when pulling 768f105 on immediate-output-when-run-on-single-thread into bfc84df on master.

[timtebeek]

Cool! So guess this means the PR as is at least fixes the immediate pretty text output for CLI and JUnit, and for TestNG anyone that needs it can come along and fix that once reported.

You mention you're unhappy about the structure; Is that something you still want to tackle in this PR, or make a note of in an item and tackle later?

I noticed there's a lot of work amassed in the develop-v5 and junit-jupiter that would touch upon some of these same classes and issues. Would it be acceptable to merge this as it and tackle them once those branches have been merged? As any refactoring now will complicate the work in those branches, where there might be more value in having those branches brought closer to a merge as compared to the unsatisfying feeling around the current implementation here.

[mpkorstanje]

The primary difference between v4 and v5 is the package structure. Refactorings have been rather trivial to port over so far. After a few do overs you get the hang of it quite quickly. So that hasn't been a bother yet.

I was planning to EOL 4.x.x in June this year to burn out the last few bugs but there has been a splurge of contributions and fixes recently. So I'm going to reset that timer. Additionally the upgrade to V5 a significant breaking change. I'd like to give the world a heads up and a chance to contribute some deprecations to allow for a graceful migration.

I'm holding of on JUnit 5 until then. I would like to overhaul Cucumbers resource loading/class path scanning first. Which will be much easier to do once we can use Java 8. By then it will probably be easier to redo the JUnit 5 implementation from scratch.

[mpkorstanje]

And to answer the question. Yes, this is something I would like to tackle.

There are a few requirements:

1. The plugins class should be created once along with all other components.
2. The plugins class should be a repository of plugin objects.
3. The plugin objects should not subscribe to the event bus in the constructor.
4. The plugin objects should be subscribed to the event bus prior to sending the first event.
5. assumeEventsInOrder is not a plugin option nor a runtime option and should be be used as such
6. Once subscribed now new plugin objects can be expected to be added to the plugins.

So I'm thinking Plugins.setEventBusOnEventListenerPlugins(bus) and Plugins.setSerialEventBusOnEventListenerPlugins(bus).

It's mostly a plumbing job.

[mpkorstanje]

We've got a slack btw. https://cucumberbdd-slack-invite.herokuapp.com/

[timtebeek]

@mpkorstanje think I've updated the MR according to your suggestions; could you check if this matches what you were aiming for?

[mpkorstanje]

Pushed some minor edits and reduced the diff via github. Lets see if the test pass. 😆

[mpkorstanje]

All passed. Thanks for putting the time into this.

[timtebeek]

My pleasure! :)

[lock]

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.