Skip to content

Conversation

arensman
Copy link
Contributor

@arensman arensman commented May 26, 2025

  • Updated no-assert-equal rule to also catch assert.equal in module hooks
  • Moved isInModule into utils (was duplicated around a few places)
  • Added before and after to recognized module hook properties
  • Added test case

Fixes #531

@bmish
Copy link
Member

bmish commented Jun 21, 2025

Just to confirm, it looks like this will be a breaking change, right? I see isModuleHookPropertyKey is used in 4 rules. Can you add a test to each of those rules if it affects them.

@arensman
Copy link
Contributor Author

arensman commented Jun 23, 2025

Just to confirm, it looks like this will be a breaking change, right? I see isModuleHookPropertyKey is used in 4 rules. Can you add a test to each of those rules if it affects them.

Yes, that's correct. It fits the category "Minor release > A bug fix in a rule that results in ESLint reporting more errors."
That change improves the rules to detect the before and after hooks. According to QUnit documentation, they have been added with 2.0 and have the same API as beforeEach and afterEach. That's why I'm positive this change is intended.

I've added tests for the other four rules. The tests in no-arrow-tests, no-qunit-start-in-tests, and resolve-async fail without this change. The test in no-setup-teardown passes with and without.

@bmish
Copy link
Member

bmish commented Jul 11, 2025

Yes, that's correct. It fits the category "Minor release > A bug fix in a rule that results in ESLint reporting more errors."

@arensman just want to double-check that you are recommending this to be considered a bug fix for a minor release? Note that I'm also open to releasing a major release soon as part of #381.

Can you rebase?

Moved isInModule into utils (was duplicated around a few places)
Added before and after to recognized module hook properties
Added test case
@arensman arensman reopened this Jul 11, 2025
@arensman
Copy link
Contributor Author

arensman commented Jul 11, 2025

@bmish Thanks, I have rebased the changes. Yes, I recommend a minor release. Though I'm also fine with moving this into the major release, if there is one scheduled soon anyway.

I am getting linter errors now, but I see the same linter errors in main without modifications on my machine. I guess it's unrelated.

@coveralls
Copy link

coveralls commented Jul 13, 2025

Pull Request Test Coverage Report for Build 16289312641

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 92.016%

Totals Coverage Status
Change from base Build 16221566995: 0.008%
Covered Lines: 1010
Relevant Lines: 1084

💛 - Coveralls

@bmish
Copy link
Member

bmish commented Jul 13, 2025

I see the lint issues in your PR but I don't see any issues on main.

@arensman
Copy link
Contributor Author

arensman commented Jul 14, 2025

Thanks for the pointer. I was confused by an error from eslint-doc-generator, which had trouble with line endings.

I have fixed the type check errors. Can you please try again?

lib/utils.js Outdated
Comment on lines 12 to 17
const NEW_MODULE_HOOK_IDENTIFIERS = [
"before",
"beforeEach",
"afterEach",
"after",
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to split this change into a separate PR? It seems there's a bug fix to no-assert-equal as mentioned in the PR title, but then there's a separate change to the hooks used by separate rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is somewhat related. Without that change, the fix in no-assert-equal would not apply to the before and after hooks.
However, I can split it up. I’ve created a separate PR (#610) specifically for the changes to the before and after hooks.

@bmish bmish added the bug label Jul 15, 2025
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bmish bmish changed the title fix: Flag assert.equal in module hooks fix: Flag assert.equal in module hooks in no-assert-equal rule Jul 15, 2025
@bmish bmish merged commit 01157b1 into qunitjs:main Jul 15, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

"qunit/no-assert-equal" does not recognize assert.equal in module hooks
3 participants