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

endSuite hook is called without matching startSuite hook #5264

Open
RedX2501 opened this issue Nov 29, 2024 · 4 comments
Open

endSuite hook is called without matching startSuite hook #5264

RedX2501 opened this issue Nov 29, 2024 · 4 comments
Labels
status: in triage a maintainer should (re-)triage (review) this issue

Comments

@RedX2501
Copy link

I'm creating this ticket due to ticket webdriverio/webdriverio#13013

The summary of the linked issue is that endSuite might be called without a matching startSuite if a describe is used that has no tests. This can happen when dynamically generating its.

I'm not sure if it is strictly necessary for hooks to always be called pairwise (start + end) but it makes coding any state dependent hooks much easier.

Here is the diff that solved my problem:

diff --git a/node_modules/mocha/lib/runner.js b/node_modules/mocha/lib/runner.js
index 60a19f0..9103d9b 100644
--- a/node_modules/mocha/lib/runner.js
+++ b/node_modules/mocha/lib/runner.js
@@ -854,14 +854,13 @@ Runner.prototype.runSuite = function (suite, fn) {
   var total = this.grepTotal(suite);
 
   debug('runSuite(): running %s', suite.fullTitle());
+  this.emit(constants.EVENT_SUITE_BEGIN, (this.suite = suite));
 
   if (!total || (self.failures && suite._bail)) {
     debug('runSuite(): bailing');
     return fn();
   }
 
-  this.emit(constants.EVENT_SUITE_BEGIN, (this.suite = suite));
-
   function next(errSuite) {
     if (errSuite) {
       // current suite failed on a hook from errSuite
@@ -903,6 +902,7 @@ Runner.prototype.runSuite = function (suite, fn) {
     // remove reference to test
     delete self.test;
 
+    debug('done(): ending %s', suite.fullTitle());
     self.hook(HOOK_TYPE_AFTER_ALL, function () {
       self.emit(constants.EVENT_SUITE_END, suite);
       fn(errSuite);

This issue body was partially generated by patch-package.

@JoshuaKGoldberg
Copy link
Member

Interesting find! Could you / someone please post an isolated reproduction we can take a look at here? One that doesn't require knowing other downstream tools like webdriverio. Thanks!

https://antfu.me/posts/why-reproductions-are-required

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Nov 29, 2024
@mochajs mochajs deleted a comment from king-2929 Dec 10, 2024
@JoshuaKGoldberg
Copy link
Member

Ping @RedX2501 - any chance you could post an isolated reproduction please?

@JoshuaKGoldberg JoshuaKGoldberg added the stale this has been inactive for a while... label Jan 2, 2025
@RedX2501
Copy link
Author

RedX2501 commented Jan 10, 2025

@JoshuaKGoldberg

After much trying out I was finally able to find a reproduction.

I don't use mocha on it's own and I have no project on which I could try. I work only on an electron project.

You must use allure reporter and testing-library for the getByText call.

import { setupBrowser, waitForMainWindow } from "../helpers.ts";
describe("Setup", () => {
  // Just some setup for my test. Probably not needed.
  before("setup", async () => {
    await waitForMainWindow();
    await browser.pause(2000);
    setupBrowser();
  });

  describe("simple", () => {
    describe("simple", () => {
      before("simple before", async () => {
        await browser.pause(100);

        // This testing-library call causes the error.
        const ele = await browser.getByText("Overview");

        // Using the equivalent wdio selector just works.
        //const ele = await browser.$("//*[contains(text(), 'Overview')]")
        await expect(ele).toBeDisplayed();
      });
      describe("deep", () => {
        it("some test", async () => {
          await browser.$("div");
          await expect(1).toBe(1);
        });
      });
    });
  });
});

package.json:

{
  "name": "eev_test",
  "type": "module",
  "devDependencies": {
    "@eslint/js": "^9.15.0",
    "@stylistic/eslint-plugin": "^2.11.0",
    "@testing-library/jest-dom": "^6.5.0",
    "@testing-library/react": "^16.0.1",
    "@testing-library/user-event": "^14.5.2",
    "@testing-library/webdriverio": "^3.2.1",
    "@wdio/allure-reporter": "^8.40.3",
    "@wdio/cli": "^8.40.3",
    "@wdio/local-runner": "^8.40.3",
    "@wdio/mocha-framework": "^8.40.3",
    "@wdio/spec-reporter": "^8.40.3",
    "@wdio/visual-service": "^5.2.1",
    "allure-commandline": "^2.32.0",
    "electron": "^29.1.4",
    "es-main": "^1.3.0",
    "eslint": "^9.15.0",
    "eslint-plugin-prettier": "^5.2.1",
    "patch-package": "^8.0.0",
    "radash": "^12.1.0",
    "ts-node": "^10.9.2",
    "typescript": "^5.7.2",
    "typescript-eslint": "^8.16.0",
    "wdio-electron-service": "^6.6.1",
    "wdio-html-nice-reporter": "^8.1.6"
  },
  "scripts": {
    "wdio": "wdio run ./wdio.conf.ts"
  }
}

Trying to figure out the problem I noticed that if you have an empty describe block (i.e. one without tests) the endSuite event is not called. It will got to the next test (startSuite).

Not sure if this is intentional.

@JoshuaKGoldberg JoshuaKGoldberg added status: in triage a maintainer should (re-)triage (review) this issue and removed status: waiting for author waiting on response from OP - more information needed stale this has been inactive for a while... labels Jan 10, 2025
@JoshuaKGoldberg
Copy link
Member

Well, thanks for trying! Putting this back in triage for when a team member has a chance to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: in triage a maintainer should (re-)triage (review) this issue
Projects
None yet
Development

No branches or pull requests

2 participants