Move events infrastructure to the core

[mattwynne]

Rationale

We're using events heavily in the front-end now, and it makes more sense to have both the Events::Bus plumbing and the firing of events come straight out of the core. As a bonus, we remove the conceptual overhead of a "report".

TODO:

• Add complete tests for the report API in spec/core_spec.rb
• Port over the tests for the event bus from cucumber/cucumber-ruby
• Get @tooky and @brasmusson's feedback on the new API - WDYT?
• Update cucumber to use the new API (see Integrate new core events cucumber-ruby#973)
• improve the YARD docs for events
• fix the readme prose to talk about events not report
• Ship it!

[mattwynne]

We should also add a GherkinParsed event or similar

[mattwynne]

@danascheider @brasmusson @tooky this PR craves your attention!

Seriously, it would be good to get this merged (and cucumber/cucumber-ruby#973 which leans on it) as it's a fairly big lump and the longer we leave it the harder it will be to do.

Would it make it easier to read if I tidy up the commits?

[mattwynne]

Since it's happened multiple times, it's probably something we should look at though

I agree. I haven't witnessed these failures myself I don't think. Can you raise an issue with the travis output in them @danascheider? I can't see how to dig out the original failure for a build that's been rebuilt.

[danascheider]

Sure, at first I thought it was just a random hiccup in Travis and not a problem with Cucumber and JRuby, so I didn't see it as necessary. I'll create an issue and then next time the failure comes up, I'll paste the error message into the issue. And if it doesn't come up, so much the better.

[mattwynne]

You can't remember which test it was that failed, can you?

[danascheider]

Yes, it was the very last one in both cases.

[brasmusson]

You mean the :slow test? Yes, normally it runs in ~7s on JRuby on Travis, but sometimes it takes slightly more than 10s - which makes it fail. Haven't we somewhere extended timeouts in test for JRuby? In Cucumber-Ruby? Maybe we should do the same here.

[danascheider]

Yes, @brasmusson - it's the slow one

[brasmusson]

This change does not work with coveralls v0.8.2, the result is a "initialize': wrong number of arguments (1 for 0) (ArgumentError)". I happen to have coveralls v0.8.2 installed as it both fulfils the cucmber-ruby(-core) requirement of '~>0.7' and the gherkin requirement of '~> 0.8', '< 0.8.8'.

[mattwynne]

You're right - this change doesn't belong in here. We should sort this out in the master branch.

[brasmusson]

Reverted in cf7e975.

[brasmusson]

If this class is connected directly to the event_bus then it need to filter out the notification from the executed hooks, since they are not included in the step counts.

[brasmusson]

Fixed in 7bbae8b

[brasmusson]

I took the step to merge this (both to make progress on this and to be able to rebase cucumber/cucumber-ruby#977 with the event bus in the core). Two review comment are not fixed (#106 (comment) and #106 (comment)), not because I reject them, but more with the comment "yes, there are duplication here, but I do not see the simple refactoring to fix that, so I choose not to spend time on working on them, they can be improved on the master branch".

[mattwynne]

Thanks for all the scrutiny and tidy-up work @danascheider and @brasmusson 👏

[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.