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

Limitation: no-assert-ok does not working for nested function #199

Open
Mifrill opened this issue Aug 6, 2021 · 1 comment
Open

Limitation: no-assert-ok does not working for nested function #199

Mifrill opened this issue Aug 6, 2021 · 1 comment

Comments

@Mifrill
Copy link

Mifrill commented Aug 6, 2021

rule enabled in .eslintrc.js:

  extends: [
    'plugin:qunit/recommended',
  ],
  // ...
  rules: {
    'qunit/no-assert-ok': 2,
  },

but the Rule ignores if assert.ok is used in shared function:

  test('test 1', async function (assert) {
    await sharedAssert(assert);
  });

  test('test 2', async function (assert) {
    await sharedAssert(assert);
  });

  async function sharedAssert(assert) {
    await visit('/');
    assert.ok(true);
  }

For example:
https://github.com/Mifrill/demo-eslint-plugin-qunit-no-assert-ok-ignore-nested-function/blob/4053e43237290754a88939e49e27732feeba24d3/tests/acceptance/index-test.js#L1-L22

Lint passed:
https://github.com/Mifrill/demo-eslint-plugin-qunit-no-assert-ok-ignore-nested-function/runs/3266269169

Rule:
https://github.com/platinumazure/eslint-plugin-qunit/blob/master/docs/rules/no-assert-ok.md

Repo with bug reproduce:
https://github.com/Mifrill/demo-eslint-plugin-qunit-no-assert-ok-ignore-nested-function

Expected lint:js failed:
Mifrill/demo-eslint-plugin-qunit-no-assert-ok-ignore-nested-function#1

@platinumazure
Copy link
Owner

This is more of a known limitation at this point.

I think we could enhance the rule to look for functions defined in the same file, and explicitly called by test functions. However, we would not be able to do this as easily with functions imported from a different file, and I'm not sure we should.

PR would be welcome. We would need to use proper variable analysis (probably via the eslint-utils package) to trace as accurately as we can.

We would need to make sure the assert variable name can be locally renamed and still caught. In other words, we need to add a unit test like this:

QUnit.test("test", function (assert) {
    assertWrapper(assert);
});

function assertWrapper(foo) {
    foo.ok(true);     // should be detected
}

I would also accept a docs PR indicating the current known limitation (assertion must be in the test itself).

@platinumazure platinumazure changed the title Bug: no-assert-ok does not working for nested function Limitation: no-assert-ok does not working for nested function Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants