Use @this to satisfy ESLint's no-invalid-this

[deyaaeldeen]

Problem

no-invalid-this makes sure uses of this are valid (see docs and implementation). However, uses of this are rampant in our test suites because this is how mocha unit tests are structured, the Mocha context can be accessed only through this.

Fix

So instead of disabling no-invalid-this in our test suites, this PR tags functions that reference this with @this and that satisfies the rule requirements (see docs).

Discussion

It could be argued that this work just replaces one comment annotation with another so we did not really solve the underlying problem. However, the inherent problem lies in how mocha itself works and there is nothing we can do other than probably migrating to another framework that is more sane/type-safe. One minor improvement we get is we now have slightly less syntactic overhead because we need to tag just the function instead of individual lines in its body that violate the rule.

Trade-offs

Pros:

• function tags are less than line tags

Cons:

• whitelisting one more tag with the tsdoc linter that our devs need to learn about
• still having rampant tags everywhere

Fixes #11404

[ramya-rao-a]

/azp run js - eventgrid - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ghost]

Hello @ramya-rao-a!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[maorleger]

I'm a little late to the party here but I am not in favor of this approach personally for the following reasons:

1. Since it's mostly test code I'd rather disable no-invalid-this in tests if the rule is not typescript-aware (i.e. this is valid, properly annotated, and properly captured by Mocha but still flagged incorrectly). For test code this really feels like noise to me.
2. In product code we will be better off addressing this case-by-case. Tagging @this feels like something I would add to make the linter happy without too much thought whereas disabling the rule as a one-off is something I would give more thought to. It also doesn't solve the underlying problem, just makes the tooling happy. this could still be invalid couldn't it?
3. wide-reaching PRs like this make rebasing a nightmare for anything that is currently in-flight.

I love the work you put into this, and I love that we're making our tooling better, but since we didn't solve the underlying problem what do I do when I have a linter error in tests, just tag @this blindly? That feels like the rule isn't adding much value when working with Mocha. Just my 2 cents though!

[deyaaeldeen]

@maorleger I feel your pain and you can see a discussion around this in the PR description. I am not really fond of either this approach or turning off the rule all together in tests. The underlying issue lies in how mocha works which is disturbing to me. I mean, look at some of the side-effects of their design: #13005.

I can be ok with turning the rule off for tests but that solution still does not solve the underlying problem. Anyhow, I do not have a strong preference either way and we should not expect to use a lot of this going forward anyway or at least we should discourage its use.

[maorleger]

@deyaaeldeen Yeah, I think it just comes with the territory - this is weird in JS (weird as in unexpected and different than other languages) and it's made worse because Mocha uses it everywhere.

One compromise / improvement we can make here is using typescript-eslint's no-invalid-this instead of eslint's version which does recognize when you pass the this "parameter" to a typescript function.

So then instead of

it("can abort creating a key", /** @this Mocha.Context */ async function() {

we would use

it("can abort creating a key", async function(this: Mocha.Context) 

And then at least it's just TypeScript without the inline JSDoc comments - what do you think? We already use typescript-eslint in a few places.

Edit, we also get the benefit of the compiler letting us know that we cannot use the this parameter in an arrow function, likely improving the experience when using it in Mocha.

[ramya-rao-a]

Sounds great to me!

Sorry for being trigger happy and merging this PR :(
Got excited to be able to enable the linter in CI now that the errors got fixed