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.

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

"hook" event is triggered after "test" event #4266

Closed
4 tasks done
sskorol opened this issue May 3, 2020 · 7 comments
Closed
4 tasks done

"hook" event is triggered after "test" event #4266

sskorol opened this issue May 3, 2020 · 7 comments
Labels
status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior

Comments

@sskorol
Copy link

sskorol commented May 3, 2020

Prerequisites

  • Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
  • Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
  • 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
  • Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

When we create a custom reporter with common events' handlers as described here, hook is always triggered after test event, that seems incorrect.

Steps to Reproduce

See a min repo to reproduce the issue.

Expected behavior: hook event is triggered before test event.

Actual behavior: hook event is triggered after test event.

Reproduces how often: 100%

Versions

  • The output of mocha --version and node node_modules/.bin/mocha --version: 7.1.2
  • The output of node --version: v13.13.0
  • Your operating system
    • name and version: macOS Catalina, 10.15.1
    • architecture (32 or 64-bit): 64-bit
  • Your shell (e.g., bash, zsh, PowerShell, cmd): zsh
sskorol added a commit to sskorol/allure-js that referenced this issue May 3, 2020
As it's hard to reuse current allure-mocha implementation within other frameworks or libraries, there were made the following modifications:
- updated package dependencies to the latest versions;
- updated typescript dependency in a root module, as re-exporting issue is already fixed;
- replaced deprecated `mocha-typescript` with `@testdeck/mocha`;
- added before/after hooks handlers (note that before hook won't work as expected until [mocha issue](mochajs/mocha#4266) is fixed or we revise the current contexts' switching approach);
- added `AllureContainer` to the `allure-js-commons` package for easier decorators' implementation;
- covered `epic` feature;
- updated all the tests to support `@testdeck/mocha`.
sskorol added a commit to sskorol/allure-js that referenced this issue May 9, 2020
As it's hard to reuse current allure-mocha implementation within other frameworks or libraries, there were made the following modifications:
- updated package dependencies to the latest versions;
- replaced deprecated `mocha-typescript` with `@testdeck/mocha`;
- added before/after hooks handlers (note that before hook won't work as expected until [mocha issue](mochajs/mocha#4266) is fixed or we revise the current contexts' switching approach);
- covered `epic` feature;
- updated all the tests to support `@testdeck/mocha`.
sskorol added a commit to sskorol/allure-js that referenced this issue May 9, 2020
As it's hard to reuse current allure-mocha implementation within other frameworks or libraries, there were made the following modifications:
- updated package dependencies to the latest versions;
- replaced deprecated `mocha-typescript` with `@testdeck/mocha`;
- added before/after hooks handlers (note that before hook won't work as expected until [mocha issue](mochajs/mocha#4266) is fixed or we revise the current contexts' switching approach);
- covered `epic` feature;
- updated all the tests to support `@testdeck/mocha`.
@craigtaub
Copy link
Contributor

Hi @sskorol .

"hook start" event is executed after "test" event

Im confused by the issue. The runner will run the before "hooks", then the "test", then the after "hooks". The hooks all have a "hook start" event, so why would "afterEach" not have one?

I see you are listening for the hook event, could it help to listen to a more specific hook event (e.g afterEach)?
Apologies if I am not understanding the actual problem.

@sskorol
Copy link
Author

sskorol commented Jun 10, 2020

Hi @craigtaub ,

Apologies if I introduced such confusion. Probably the following screenshot would explain the issue better:

image

What I've been talking about is "before each" hook's execution order. I expect to see a test event fired only when before hooks are finished. But in the console log we see that a first fired event = test event, which confuses a custom reporter's behavior.

P.S. Sorry, I didn't get what you meant by "listen to a more specific (afterEach) hook event". I don't need afterEach, I need beforeEach hook triggered before test, and not after (as on the screenshot). While building a custom Mocha reporter it's important to have a correct hooks' execution order:

  • before each hook start
  • before each hook end
  • test start (*)
  • test pass/fail/skip
  • after each hook start
  • after each hook end

For now, test event is triggered before all the other hooks. That's my issue. Please let me know if I missed something and there are some "hidden" hooks (triggered before test) I may want to take a look at, and which are not listed in the official Mocha Wiki.

@craigtaub
Copy link
Contributor

craigtaub commented Jun 14, 2020

Yes I see what you mean.

The runner fires EVENT_TEST_BEGIN followed by the beforeEach hooks and the test. After that EVENT_TEST_END fires followed by the afterEach hooks.
From this it does seem that "test execution" includes beforeEach but not afterEach, which seems confusing. AFAIK it should include both hooks (according to "run cycle overview").

It appears only the "list" reporter listens for EVENT_TEST_BEGIN event so it is possible this could be a bug which has flown under the radar. Although TEST_END is used much more.
I am really unsure here and feel like I am missing something as the event order is pretty critical and has not changed.

@mochajs/core any thoughts on this?

@juergba
Copy link
Contributor

juergba commented Jun 15, 2020

This behaviour is not a bug, it was designed like this back in ancient times. We have integration tests for checking the event order, and there already are similar issues.

This is not a trivial topic, there should be good reasons for a change. It affects at least:

  • the failing hook pattern (test is skipped upon failing beforeEach)
  • conditional this.skip() in beforeEach
  • when a test passes and then its afterEach fails, Mocha does not fail the test retrospectively.

@craigtaub
Copy link
Contributor

craigtaub commented Jun 15, 2020

Thanks.
I'll see if I can find an existing issue.

I need beforeEach hook triggered before test, and not after

@sskorol unfortunately this is an outstanding nontrivial bug which has not been resolved yet. Perhaps there is an alternative approach you can take?

@sskorol
Copy link
Author

sskorol commented Jun 16, 2020

@craigtaub @juergba the main problem I see here is that such a design flaw on the core level may force API users to create workarounds for all potential child reporters. This issue won't be annoying unless you need to collect some internal metadata from within the hook's body and physically save or send it somewhere in real-time. In this case, the wrong execution order might affect the child reporter's architecture or cause misinterpreting results by the receiver side. Of course, workarounds are always possible. But I would be greatly appreciated if such issues are considered for fixing on the core level.

@JoshuaKGoldberg
Copy link
Member

Per #5027, we're not looking to make any major changes to Mocha at this point.

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg added status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior and removed unconfirmed-bug labels Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wontfix typically a feature which won't be added, or a "bug" which is actually intended behavior
Projects
None yet
Development

No branches or pull requests

4 participants