Config/preset changes for 5.0.0

[platinumazure]

Here are all of the non-recommended rules:

• no-arrow-tests
• no-assert-equal
• no-assert-logical-expression
• no-assert-ok
• no-async-test
• no-compare-relation-boolean
• no-conditional-assertions
• no-early-return
• no-global-assertions
• no-global-expect
• no-global-module-test
• no-global-stop-start
• no-init
• no-jsdump
• no-negated-ok
• no-qunit-push
• no-qunit-start-in-tests
• no-qunit-stop
• no-setup-teardown
• no-skip
• no-test-expect-argument

Which rules should be added to qunit/recommended?

[platinumazure]

Here are the ones I think are worth adding (just from my perspective):

• no-assert-logical-expression
• no-conditional-assertions
• no-early-return
• no-only (was no-skip, but that was a typo)

[ventuno]

I'm very partial to:

• no-assert-equal;
• no-assert-ok.

[platinumazure]

Thanks @ventuno!

I should have added in my original post that I really want qunit/recommended to mainly contain rules that nearly everyone would agree are worth turning on. I'm worried that no-assert-equal and no-assert-ok could be contentious, and that we might have just as many people overriding them to be turned off in their configurations compared to the number of people who currently need to override them to be turned on today. If a large percentage of users have to add two extra lines to their configuration, that's not ideal. So, my preference is to add rules that help with clear errors or difficult-to-maintain code as much as possible, and allow users to add stylistic or preference-based rules to their own configuration files as needed.

I might be open to creating a new configuration, for example "qunit/strict", which could contain recommended rules plus rules that forbid loose assertions or other preference-based rules. That would be a new feature rather than a breaking change, so it could be added in a minor release. If you like this idea, please feel free to open a new issue with a proposal of which rules make sense to add to a new "strict" exported configuration.

With all of that said, if a number of users express clear support for adding those rules to qunit/recommended, then I would probably need to bow to the community's wishes. 😄

[ventuno]

Thanks @platinumazure, I agree with you these would be rather contentious, and wasn't really expecting them to be picked up. I figured I'd at least put the option out there and see what people thought :-).

[edg2s]

I'm not sure I would include no-skip. A skipped rule is like a TODO and it the fact that the developer wrote ".skip" document this and the developer's intention, so requiring an additional eslint-disable comment seems excessive.

Certainly many will want to enable this rule, but I don't see it as being universal enough to include in recommended.

[platinumazure]

I'm not sure I would include no-skip. A skipped rule is like a TODO and it the fact that the developer wrote ".skip" document this and the developer's intention, so requiring an additional eslint-disable comment seems excessive.

Certainly many will want to enable this rule, but I don't see it as being universal enough to include in recommended.

Actually, on reading this, I agree. I think I was thinking of no-only in my head for some reason. no-only is probably good to include because valid tests (that is, ones not marked QUnit.only) could be skipped from running in CI and that's probably not what people want. But QUnit.skip is meant to be a documented skip case-- individuals may want to use the rule but it's not universal enough.

Thanks @edg2s for the feedback!

[platinumazure]

Hi everyone! After a long time, I think I'm about ready to release eslint-plugin-qunit 5.0.0. The only thing left is the recommended preset.

Based on everyone's feedback, this is what I would like to do:

• Add the following rules to recommended config:
  • no-assert-logical-expression
  • no-conditional-assertions
  • no-early-return
  • no-only
• Agree in principle to the creation of a new strict config, but that can be done in a minor release and does not have to be ready in 5.0.0. This could potentially include no-assert-equal, no-assert-ok, or other rules. This should be discussed in a separate issue.

Please speak up this week if you have any concerns with this plan or want me to consider other rules for recommended. Once we have consensus (or at least no objections), I'll implement the changes and release 5.0.0!

[Krinkle]

• no-assert-logical-expression
• no-conditional-assertions
• no-early-return
• no-only

These all LGTM to recommend by default.

I agree that no-assert-equal can be contentious and isn't super important. I feel a bit more strongly about no-assert-ok, but I can see it fitting better with a stricter preset.

There's probably a few less contentious things we could include as a subset of that, such as no-negated-ok and no-ok-equality. Of these, I see no-ok-equality is already in the recommended preset.

I wouldn't mind adding both, but perhaps it'd be less controversial if they have an auto-fixer before we do, so perhaps not for now.

[platinumazure]

@Krinkle I think the reason I had held off on adding no-negated-ok to recommended originally was because assert.notOk was a relatively recent addition to QUnit at the time the rule was created. Enough time has passed that we could probably add the rule to recommended.

That said, I could also see an argument that it's more stylistic than anything else, and should instead go in a strict preset. What would you think if we put it there for now, and maybe consider promoting to recommended later?

To me, no-ok-equality is a different case than no-negated-ok because when you use an equality comparison in an assert.ok, you lose the ability to see an expected value. But for no-negated-ok, there is no expected value (besides truthy or falsy) whether you use assert.ok or assert.notOk, so I don't see this as a case where useful information is lost. The information loss is why I think no-negated-ok makes sense as a recommended rule rather than a member of a strict preset.

What do you think?

[Krinkle]

When using assert.ok( !value ) the actual value is lost from the display. While there is no expected value, QUnit does display the actual value in full, which is not as meaningful when negating it first in the caller. I would also argue that it makes test results harder to understand because there is a level of negation indirection on the value that is actually generated. I would assume that !value is primarily used where the author couldn't yet use notOk or didn't know about it.

Either way, I agree that it can be done later, e.g. based on adoption and feedback of the strict preset.

[platinumazure]

I forget that the actual value is still displayed. In any case, we seem to agree no-negated-ok can wait to be added to recommended for now, so that's the direction we'll go for 5.0.0. Thanks for the discussion!