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

🐛 Bug: Watch + mochaHooks inconsistent state on re-runs #5149

Open
4 tasks done
Kartones opened this issue May 28, 2024 · 3 comments
Open
4 tasks done

🐛 Bug: Watch + mochaHooks inconsistent state on re-runs #5149

Kartones opened this issue May 28, 2024 · 3 comments
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer
Milestone

Comments

@Kartones
Copy link

Kartones commented May 28, 2024

Bug Report Checklist

  • I have read and agree to Mocha's Code of Conduct and Contributing Guidelines
  • I have searched for related issues and issues with the faq label, but none matched my issue.
  • I have '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, my usage of Mocha, or Mocha itself.
  • I want to provide a PR to resolve this

Expected

Mocha's watch mode should produce the same results on all runs, if the code does not change significantly (e.g. triggered by adding a new line.). Using mochaHooks that mutate globals and modules/requires should be consistent in all runs.

  Failing on watch reload
mochaHooks beforeEach(): [ 'testItem1', 'testItem2' ]
    ✔ should return two items

  Always working
mochaHooks beforeEach(): [ 'testItem1', 'testItem2' ]
    ✔ should return two items

  2 passing (2ms)

Actual

Mocha's watch mode seems to not work the same on reloads regarding mochaHooks as in the 1st run.

Having a beforeEach() that requires a fil, and which we mutate state of in that hook, when a test that relies on that mutated state runs, first time is ok, but subsequent "watch runs" fail.

First run is ok, but then any irrelevant change on a watched file:

  Failing on watch reload
mochaHooks beforeEach(): [ 'testItem1', 'testItem2' ]
    1) should return two items

  Always working
mochaHooks beforeEach(): [ 'testItem1', 'testItem2' ]
    ✔ should return two items

  1 passing (13ms)
  1 failing

  1) Failing on watch reload
       should return two items:

      AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:
[]

should loosely deep-equal
[
  'testItem1',
  'testItem2'
]
      + expected - actual

      -[]
      +[
      +  "testItem1"
      +  "testItem2"
      +]

      at Context.<anonymous> (test/failing.test.js:8:12)
      at process.processImmediate (node:internal/timers:478:21)


ℹ [mocha] waiting for changes...

Minimal, Reproducible Example

I created a small repro repo, keeping it similar to my source: https://github.com/Kartones/mocha-hooks-watcher-bug

Includes a README with steps to reproduce, but adding here a minimal TL;DR:

given:

class A {
  _items = [];

  initialize(items) {
    this._items = items;
  }

  getItems() {
    return this._items;
  }
}

exports.A = A;
exports.a = new A();

And a hook that mutates it:

const { a } = require("../lib/a.js");

exports.mochaHooks = {
  beforeEach() {
    a.initialize(["testItem1", "testItem2"]);
  },
};

The hook is correctly executed before each test, but a seems to be kept at the initial state (with _items=[], instead of _items=["testItem1", "testItem2"])

Versions

Mocha: 10.4.0
NodeJS: v20.11.1
pnpm (not strictly relevant): pnpm@8.15.3

Additional Info

UPDATED: Taking a look, will provide a PR with one solution.

@Kartones Kartones added status: in triage a maintainer should (re-)triage (review) this issue type: bug a defect, confirmed by a maintainer labels May 28, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the semver-major implementation requires increase of "major" version number; "breaking changes" label Jul 3, 2024
@JoshuaKGoldberg
Copy link
Member

Fantastic investigation, thanks! I've gone through the (very helpful!) reproduction and confirmed the bug. +1 that only clearing some of the file cache is a root cause bug.

Unfortunately, I think that fixing the behavior by clearing more of the cache would be considered a semver-major breaking change. It's likely that some users have come to depend on this odd caching behavior.

cc @mochajs/maintenance-crew for a second or third opinion, just to be safe.

@Kartones
Copy link
Author

Kartones commented Jul 7, 2024

Thanks for checking it out!

Bug fixed, now doing a full blast, and no problem in considering it a major due to potential breakages 👍

@JoshuaKGoldberg JoshuaKGoldberg added status: accepting prs Mocha can use your help with this one! and removed status: in triage a maintainer should (re-)triage (review) this issue labels Oct 8, 2024
@JoshuaKGoldberg JoshuaKGoldberg added this to the v12.0.0 milestone Oct 8, 2024
kirrg001 added a commit to instana/nodejs that referenced this issue Nov 11, 2024
@kirrg001
Copy link

Thanks for the investigation. We are also running into this problem. This has helped us as a workaround.
Thank you so much!

kirrg001 added a commit to instana/nodejs that referenced this issue Nov 12, 2024
kirrg001 added a commit to instana/nodejs that referenced this issue Nov 12, 2024
- Added node script to add terminal aliases (helps to easily run test groups)
- Integrated mocha watch
- Removed redundant sh helper scripts
- See mochajs/mocha#5149

---------

Co-authored-by: Arya <90748009+aryamohanan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major implementation requires increase of "major" version number; "breaking changes" status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@kirrg001 @Kartones @JoshuaKGoldberg and others