add global hooks

[davidjgoss]

🤔 What's changed?

• Add new messages for global hooks starting and finishing
• Add id to TestRunStarted so that the corresponding finish event, test cases and global hooks can be tied back to it
• Split out TestStepResult into its own file

⚡️ What's your motivation?

Per #66 we want to represent global hooks in the messages, because currently if they fail (thus causing the test run to fail) they aren't visible in reporting.

Putting an id on TestRunStarted is an approach we've discussed before and roughly agreed on in the context of global attachments per cucumber/cucumber-js#1394 (comment) - this PR doesn't address the attachments problem directly, but does lay the groundwork for it, and also means it starts to look more possible to have multiple test runs in the same stream of messages and be able to differentiate them. Note that Gherkin messages and support code aren't linked to a test run because they can be reused - it's TestCase that has the link.

🏷️ What kind of change is this?

• 💥 Breaking change (incompatible changes to the API) (because id is required on TestRunStarted)

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

---

This text was originally generated from a template, then edited by hand. You can modify the template here.

[davidjgoss]

A bit of a chicken and egg problem with the perl tests here - they rely on the current published version of the CCK, whose messages don't include the new field, which is causing the failure. Any thoughts on how to address this @ehuelsmann?

[mpkorstanje]

A bit of a chicken and egg problem with the perl tests here - they rely on the current published version of the CCK, whose messages don't include the new field, which is causing the failure. Any thoughts on how to address this @ehuelsmann?

@ciaranmcnulty looks like the acceptance tests against the CCK are not invoked for PHP:

There were 2 skipped tests:

1) AcceptanceTest::testAllNdJsonSurvivesDecodingThenEncoding
Test for AcceptanceTest::testAllNdJsonSurvivesDecodingThenEncoding skipped by data provider
PHPUnit\Framework\SkippedTestError: <no message>

2) AcceptanceTest::testAllFileStreamsSurviveDecodingThenEncoding
Test for AcceptanceTest::testAllFileStreamsSurviveDecodingThenEncoding skipped by data provider
PHPUnit\Framework\SkippedTestError: <no message>

[mpkorstanje]

A bit of a chicken and egg problem with the perl tests here - they rely on the current published version of the CCK, whose messages don't include the new field, which is causing the failure. Any thoughts on how to address this @ehuelsmann?

@davidjgoss looking at the test I think we can change them a little so. So instead of comparing the round trip ndjson --> object --> ndjson and comparing the result we can do object --> ndjson --> object and compare that result. That way we don't need the CCK at all and the code will change along with that specific message object.

Though I think it would be even better to include a testdata folder with (for now) a single message that all languages try to round trip. That way we won't have to update N implementations manually.

[mpkorstanje]

What is the rationale for making the field optional? I don't think there is a sensible behavior for a consumer that needs this field when it is absent.

[davidjgoss]

If we make it required now, the new code will be immediately incompatible with messages generated from the old code. This means e.g. we'd need to wait for all implementations to add this before we could use the new messages version in formatters.

If we're fine with that then I'm happy to change. But it seemed better to add it as optional, and maybe circle back and make it required down the line once it's established everywhere.

[mpkorstanje]

Okay, that's acceptable. Can you make a ticket so we won't forget to make it required?

[davidjgoss]

Done #113