search

platinumazure / eslint-plugin-qunit

ESLint plugin containing rules useful for QUnit tests.
MIT License
30 stars 22 forks source link

V7 Planned Changes #175

Closed bmish closed 3 years ago

bmish commented 3 years ago

Starting a list of my desired changes, feedback welcome.

For reference, V6 was release 2021-03 (CHANGELOG, discussion in #114).

platinumazure commented 3 years ago

I'd like to add renaming no-negated-ok and no-ok-equality rules. We could start by copying the rule into a new name/file, marking the existing rules deprecated in a major release, and removing the old rules in the next major.

Also, I believe there's another rule which has made other rules redundant already, but I'll have to dig that up.

platinumazure commented 3 years ago

So I'm thinking with Node 15 near end of life, and with a few dependencies having already cut off support for Node 10, I would like to cut a major version in early June with those Node versions removed from our supported engines and those dependencies upgraded. And we can pull in whatever else is ready at that point.

@bmish What do you think?

platinumazure commented 3 years ago

I've created a 7.0.0 project for tracking progress a little more easily. Let's use that instead of tracking manually in this issue.

The project will generally only track breaking changes and other changes we want to block 7.0.0. Semver-minor and semver-patch changes will be decided on a case by case basis. Contributors are encouraged to work on any semver-minor or semver-patch issue important to them, and we will release those changes when they are ready (subject to ease of logistics), which could mean before or after the 7.0.0 release.

If anyone has anything they want me to consider adding to the project, comment here or ping me in the relevant issue/PR.

cc @bmish

Krinkle commented 3 years ago

What do you think about possibly removing no-arrow-tests from the "recommended" set?

Over the past year, I have updated most upstream docs and examples to adopt arrow functions in code examples aimed at modern/ES6 browsers (the classic syntax examples continue to use regular functions of course).

I understand that in order to set properties on the this context from a hook, or when using such properties inside a test function, one will need to use a regular function for that to work. And this would naturally prevent a test from passing during development if not done correctly.

In my view, the use of this is a legacy pattern from the QUnit 1-era, predating scoped modules (added in QUnit 1.20). I tend to recommend use of local variables instead, which are in my experience easier to understand and deal with. It is less surprising to newcomers when you assign a variable from a "before" hook, that this is not somehow cloned or reset. It is entirely in your control. Plus they tend to play a bit better with IDEs as they are naturally the same logical variables throughout your test suite, without the IDE somehow needing to know that each of the various "this" objects are kind-of-but-not-really sharing a common parent object.

I have no intention of deprecating or otherwise discouraging this pattern in any way, but I do think it'd be better for new users if we don't discourage use of arrow functions going forward.

platinumazure commented 3 years ago

@Krinkle At this point, I could get behind removing that rule from the recommended set.

@bmish Do you have any strong opinions either way on removing no-arrow-tests from the recommended ruleset?

raycohen commented 3 years ago

My disagreement to @Krinkle here probably boils down to the balance of getting users onboarded with as simple concepts as possible versus preventing people from running into a problem where they need the extra capabilities of using module/test context as a file/module/project grows. There are useful patterns that can only be accomplished with module/test context, and if you eventually want to use one in a big module you've filled with arrow functions, you may have a lot of manual adjustments to make. You can get into the situation where some of your test files are set up to be able to use module context and some don't, meaning you have to mentally switch between the two setups. I also think there are footguns for a long-lived test file when you rely on lexical scoping - variables can be referenced outside of where they were originally intended to be referenced which is easy to miss as a developer and during code review. With module/test context, a variable won't be defined if you didn't set it up in the hierarchy

Krinkle commented 3 years ago

@raycohen I agree there can be benefits to the context pattern vs lexical scope pattern for shared variables, however I don't think any use case requires one pattern over the other in order to work. I'm not entirely sure if it's true, but I suspect that for any test that is passing and working as intended, one can swap out the line-for-line without changing or adding anything and it still pass and behave the same.

The lexical pattern has as benefit that declaring/initialising upfront is naturally required (and otherwise likely resulting in runtime errors or lint warnings for undeclared variables), which seems to make it more likely that a developer will remember to reset/initialise them correctly in a beforeEach hook.

The context pattern has as benefit that one (intentionally or unintentionally) forget to clear the variables and they will naturally get wiped away at the end of the test by virtue of each test having a new object.

I have found that the context pattern to be harder to understand and explain for developers and have as such stopped recommending it to people by default. Individual projects can still use it of course, but I think maybe that's where the responsibility of a default preset ends, and project-specific conventions begin. In any given organisation (or repo), one can add this rule if you are familiar with and prefer the context pattern. I realize this gives a slight bias toward the lexical pattern (since it doesn't require a rule override to adopt), but it's not like we're requiring the use of arrow functions either. I imagine anyone using the lexical pattern with a modern build target would likely want to enforce that for consistency and thus enable a rule of sorts for that as well.

Having said all that, while I've seen numerous small projects adopt lexical scopes, the biggest projects using QUnit seem to all still be on context vars (including the ones I work on at Wikimedia), so it's possible there are additional hurdles I've not yet run into. I'd like to talk more on this and learn about more experiences here, and would also like to take what we learn from that and write it up in qunitjs.com materials. But.. I don't want to take up too much space in the V7 ticket. Perhaps https://gitter.im/qunitjs/qunit or https://gitter.im/platinumazure/eslint-plugin-qunit would be a good venue.

variables can be referenced outside of where they were originally intended to be referenced […] With module/test context, a variable won't be defined if you didn't set it up in the hierarchy.

It sound like this might be one of those hurdles, but I didn't quite understand the scenario you're referring to. In my mind these are actually reversed. Using a lexical variable that isn't set up explicitly as shared variable will throw a ReferenceError or an eslint warning for undeclared local variable, whereas context properties are more tolerating of ad-hoc properties being set in a subset of tests only without declaring them ahead of time. (On the other hand, during teardown the reverse is true in that context properties don't require a teardown if it's only about dereferencing to avoid accidental reuse. But then again, if a test is working as intended it seems unlikely that it would be relying on variables being torn down to behave correctly.)

@raycohen For the immediate choice of which rules we enable in the qunit:recommended preset — Would you agree with the assessment that both patterns have benefits, that either can be used with relative ease to fulfull a project's testing needs, and that not enabling the rule would be more neutral than enabling it? (To be clear, I think it's possible that we agree on this and still decide to enable the no-arrow rule. It would mean we're deciding not to be neutral and instead recommend the context pattern. As long as that's intentional, I wouldn't mind.)

raycohen commented 3 years ago

@Krinkle appreciate your thoughts. The rule being enabled does not prevent users from choosing to exclusively use lexical scope, so I feel like turning the rule on is not very far from neutral. All of the arrow function examples in the documentation would work the same if converted to non-arrow functions, right? It's not symmetrical with a hypothetical require-arrow-tests rule that would prevent the use of context.

That said if the qunitjs.com documentation is nearly all using arrow functions, I agree that defaulting no-arrow-tests to be on is fairly awkward. And perhaps we should hold off just because of that.

As for capturing all the details of what patterns are possible and what hiccups each approach can run into, I agree that is out of scope for this ticket. My "variables can be referenced outside" quote is related to your "forget to clear" statement - if you move an entire test referencing a lexical variable out of a module that sets that variable in a beforeEach hook, it can still read the value provided the test runs after that module and the variable isn't cleared. So if you refactor a test file and move something around, you can end up with an unintended test ordering dependency.

platinumazure commented 3 years ago

As long as there remain definite benefits/use cases to the test context approach, I tend to agree with @Krinkle that we shouldn't put it in a recommended ruleset.

There are a few other rules that are clear best practices, but also opinionated (and using the "other" pattern is valid), though I don't have that list at this time. It might be better to establish a new preset which is either a superset of recommended, or can be combined with recommended, in order to cover those more opinionated rules. I feel no-arrow-tests may belong in this opinionated but best practice category, and should therefore be removed from recommended for now.

Does anyone have a strong objection? Note that you would effectively need to defend the statement, "no-arrow-tests is such a clear best practice that everyone who uses this package in the most common way (using recommended ruleset) should have to do the extra work of explicitly disable this rule for their codebase if they don't want to follow this pattern".

platinumazure commented 3 years ago

Just a quick update, I'm alive and in decent mental shape but have been extremely busy. I was also out of town for a few weeks in late July. Hoping to revisit this in the next week or two.

bmish commented 3 years ago

I don't have a strong opinion about the above debate. But PRs are open for all other breaking changes needed in the v7 release, so the release has a green light from me.

platinumazure commented 3 years ago

I had thought we were nearly ready to go with the release, but we have a couple of open items that could hold things up a bit longer. How should we proceed?

no-assert-equal recommended and deprecated?

Making no-assert-equal recommended and/or deprecating no-assert-equal and no-assert-ok in favor of no-loose-assertions: We could easily make no-assert-equal recommended, but it doesn't look good for us to make a rule recommended and then deprecate it shortly afterwards.

And as @bmish noted, there are a couple of issues with no-loose-assertions that makes it harder to fully deprecate the other rules at this point.

One option is to just do the necessary work in v7.0.0. This will take some time, but may not be all that bad since we could also wait for ESLint v8 to be finalized and make sure we have no breaking changes from that project (although it does seem unlikely at this point).

What do you think we should do here?

Renaming rules that now apply to ok and true/false

I have a card in the project: "Rename no-negated-ok and no-ok-equality rules". The goal here is to rename rules which used to apply only to ok (and maybe notOk), but were retrofitted to support assert.true and assert.false.

I don't think we can just do a rename in 7.0.0. My preference would be to copy the rules with new names, and mark the old ones as deprecated. We can removed in a later major release.

I do think the copy/deprecation could be done outside of a major release, but we may want to wait for more than one major release to fully remove.

So I propose to remove this card from the 7.0.0 project, and I can create an issue for tracking later. What do you think?

Please share any thoughts you have on the above areas, as well as let me know if I'm not tracking something I should be. Thanks! (And please remember that I'm relying on the project as the real source of truth here-- we can discuss in this issue, but ultimately, the project shows what is on the roadmap for 7.0.0 at any given time.)

bmish commented 3 years ago

@platinumazure My preference:

  1. Proceed with making no-assert-equal recommended in v7 and do not deprecate any rules at this time. no-loose-assertions needs some love still, and we could target making it recommended in v8. This allows us to proceed with a more incremental approach, since no-assert-equal will help guide users on the path to eventually adopting the even stricter rule no-loose-assertions.
  2. I don't think we should necessarily rename no-negated-ok and no-ok-equality just because we added true and false coverage to them. Renaming rules causes churn for users and creates a logistical hassle for us. And I don't think that the occasional addition of new assertions (i.e. true and false) justifies renaming all related rules. We don't need to mention all affected assertions in every rule name. However, in general, I do think we can consider renaming rules if the existing names are confusing or if we come up with significantly better names, but I'm not sure that applies here.
platinumazure commented 3 years ago

@bmish

Sounds good re: no-assert-equal. I've closed my deprecation PR. I think we're ready to go with the change to recommended.

Re: renaming ok rules: My goal would be to find new rule names that covers all the assertions without explicitly naming them. I was thinking names like no-boolean-assertion-equality and no-negated-boolean-assertion. Those would hopefully be future-proofed against further renames. With all that said, however, this work doesn't belong in the 7.0.0 release, so I've removed the card from the project.

I think we're ready to go with all breaking changes. Would you mind taking a quick look through the 7.0.0 project and the open issue and PR list and let me know if you think I missed anything? If we're good to go, then I can hopefully start merging the breaking changes.

bmish commented 3 years ago

@platinumazure I like those new rule names. I would be fine if we want to rename them in this release, but also fine if we want to wait to the next release. I personally don't think it's necessary to deprecate the old rules first--I would rather just do a hard-cutover to the new rule names to avoid the deprecation logistics. Deprecating the old rule names doesn't really help anyone, since either way they still just need to do a find-and-replace for the rule names if they mention the rule names anywhere in their codebase.

I believe everything is ready for this major release.

platinumazure commented 3 years ago

I've published eslint-plugin-qunit@7.0.0-rc.0 to npm, and pushed the commits and tags to GitHub.

Using a release candidate means we have time to fix any unexpected issues, as well as potentially letting us adapt to ESLint 8.x breaking changes if any arise.

platinumazure commented 3 years ago

Closing as we have just published eslint-plugin-qunit@7.0.0!

bmish commented 3 years ago

Thanks @platinumazure!

Not sure if you've considered it, but I would recommend creating a GitHub Release for v7 containing the full v7 changelog as the description. That would help increase the visibility of the release, especially for people who subscribe/watch release notifications for this repository.

platinumazure commented 3 years ago

Thanks for the feedback @bmish, that's a good idea. I'll have to look into that. I would love to automate that sort of thing going forward.

bmish commented 3 years ago

I would definitely recommend release-it for automating the whole release process including creating the GitHub release.

I personally use release-it-lerna-changelog (a release-it plugin) myself which is based off github PR tags for categorizing changes (example changelog).

But regardless, we could still manually create the GitHub release for v7 now since it's a big release.

platinumazure commented 2 years ago

@bmish Published a GitHub release a few days ago (only now following up, sorry). I've also created issue #220 to track exploring release-it. Not sure when I'll have time to look into it but it's at least tracked. Thanks for the suggestions!