Emit appropriate messages on @BeforeAll and @AfterAll hooks

[HardNorth]

Is your feature request related to a problem? Please describe.
It look like @BeforeAll and @AfterAll hooks do not fire any events, at least I don't see it in the code explicitly.

This makes impossible to report them in third-party report systems. At least in a straightforward way, without hacking that with AspectJ.

Describe the solution you'd like
Fire TestStepStarted event as you do for all other hooks.

[mpkorstanje]

@aslakhellesoy I don't think Cucumber currently has a message type suitable for before all/after all hooks.

Using TestStep{Started,Finished} would be inappropriate because there is no testCaseStartedId.

[aurelien-reeves]

From the perspective of cucumber-ruby, as documented here: https://github.com/cucumber/cucumber-ruby/tree/main/features/docs/writing_support_code/hooks#beforeall-and-afterall, BeforeAll and AfterAll hooks are not part of the execution of the tests. There are suppose to setup and clean-up things that are not related to cucumber.

As @mpkorstanje said, I am not sure there would be an appropriate event for those.

We could maybe add new ones dedicated to such hooks?

[aslakhellesoy]

I think we would need new message types in https://github.com/cucumber/common/tree/main/messages/jsonschema - maybe BeforeAllHookStarted and BeforeAllHookFinished.

[mpkorstanje]

That would mean adding four messages {Before,After}AllHook{Started,Finished}. For regular hooks we don't even make that distinction. There is a lack of consistency there that bothers me.

[mattwynne]

What kind of reporting do you want to be able to do @HardNorth?

[HardNorth]

@mattwynne I'm not really sure what you asking. I am a developer of Report Portal. It collects all test information (structure, hooks, logs, etc.) And analyse it with a taste of AI. I implemented numerous of test framework integrations for our project. Without these hooks reporting we won't be able to reflect them properly in our application, hence if such hook failed it won't be counted which cause statistics issues. And these hooks are also invisible for our analyser as for now

[aslakhellesoy]

That would mean adding four messages {Before,After}AllHook{Started,Finished}. For regular hooks we don't even make that distinction. There is a lack of consistency there that bothers me.

Good point. Perhaps it would be better to simply have HookStarted and HookFinished. We could have a property indicating what kind of hook it is, e.g type = “BeforeAll” | “BeforeFeature” | “BeforeScenario” | “BeforeStep” | ….. If we go down this route we should also consider replacing the use of TestStepStarted and TestStepFinished for regular (scenario) hooks.

[mattwynne]

In my mind, the original {Before,After}{Scenario,Step} hooks are just special cases of TestSteps since if they fail they can fail a test case. However a BeforeAll hook isn't part of any specific test case, so it feels like a different beast.

I'm curious how you were thinking about reporting on it @HardNorth. Would a failure of a BeforeAll hook be counted as a failure of all the tests in the run? Or do you count these sorts of hooks as separate to the tests themselves?

[mpkorstanje]

The equivalent of a special test step for before hooks would be a special test case for before all hooks.

And a failure of a before all hook would be equivalent to failing the test run (same as with failure of a test case, worst result wins).

[HardNorth]

@mattwynne We don't report all tests as failed usually, just a single Item which failed. But for those who want to be extremely accurate I leave a public empty method, which can be overridden.
But without any event we won't even report a failed test run. That will be confusing for our users.

[aurelien-reeves]

Good point. Perhaps it would be better to simply have HookStarted and HookFinished. We could have a property indicating what kind of hook it is, e.g type = “BeforeAll” | “BeforeFeature” | “BeforeScenario” | “BeforeStep” | …..

+1

If we go down this route we should also consider replacing the use of TestStepStarted and TestStepFinished for regular (scenario) hooks.

+1 keeping in mind to add a proper deprecation notice before totally replacing those

[aslakhellesoy]

@aurelien-reeves I am not suggesting we deprecate TestStepStarted and TestStepFinished.

I am suggesting we use those messages only for Gherkin steps, and not for hooks.

[aurelien-reeves]

@aurelien-reeves I am not suggesting we deprecate TestStepStarted and TestStepFinished.

I am suggesting we use those messages only for Gherkin steps, and not for hooks.

Yes, I understood 👌

Still, not using TestStep{Started|Finished} anymore with hooks remains a breaking change.

[aslakhellesoy]

Yes it would be a breaking change, but there isn't anything to deprecate.

[davidjgoss]

This came up recently when looking at BeforeAll/AfterAll hooks in cucumber-js. Because these hooks and their results aren't represented in messages, there's no way for a formatter to know what happens with them, resulting in things like the formatter saying everything went fine when in fact an AfterAll hook errored and caused the test run to fail.

I tend to agree with @mattwynne's earlier comment that these global hooks are a different beast then scenario-level ones. It would be good to have the different types of hooks represented in a consistent way in the schema, but I don't think it's worth making a breaking change for (especially now the community is building tools around this stuff). I'd vote for new GlobalHookStarted and GlobalHookFinished messages as a way to solve this.

[mpkorstanje]

I'd vote for new GlobalHookStarted and GlobalHookFinished messages as a way to solve this.

Sounds good to me.

[mattwynne]

I have only just parsed, and do like @mpkorstanje's suggestion earlier that:

The equivalent of a special test step for before hooks would be a special test case for before all hooks.

And a failure of a before all hook would be equivalent to failing the test run (same as with failure of a test case, worst result wins).

We could model a BeforeAll hook by creating an additional TestCase and TestStep for it in the message stream. I like this because it would keep the protocol simple and backwards-compatible. I have a niggling concern about whether the semantics would be clear enough, but it seems like it would be the least disruptive thing to try first.

[davidjgoss]

I don't see that as being backwards-compatible, because TestCase requires a pickleId, which we won't have if it's a synthetic test case.

[mattwynne]

I don't see that as being backwards-compatible, because TestCase requires a pickleId, which we won't have if it's a synthetic test case.

True, I didn't spot that.