Skip to content

Commit

Permalink
Use @this to satisfy ESLint's no-invalid-this (Azure#14406)
Browse files Browse the repository at this point in the history
# Problem

`no-invalid-this` makes sure uses of `this` are valid (see [docs](https://eslint.org/docs/rules/no-invalid-this) and [implementation](https://github.com/eslint/eslint/blob/8984c91372e64d1e8dd2ce21b87b80977d57bff9/lib/rules/utils/ast-utils.js#L900)). 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](https://eslint.org/docs/rules/no-invalid-this)).

# 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 Azure#11404
  • Loading branch information
deyaaeldeen authored and vindicatesociety committed Apr 26, 2021
1 parent cf0249a commit 0c3f72f
Show file tree
Hide file tree
Showing 101 changed files with 935 additions and 802 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ describe("AnomalyDetectorClient", () => {
let recorder: Recorder;
const apiKey = new AzureKeyCredential(testEnv.ANOMALY_DETECTOR_API_KEY);

beforeEach(function() {
// eslint-disable-next-line no-invalid-this
({ recorder, client } = createRecordedAnomalyDetectorClient(this, apiKey));
});
beforeEach(
/** @this Mocha.Context */ function() {
({ recorder, client } = createRecordedAnomalyDetectorClient(this, apiKey));
}
);

afterEach(async function() {
if (recorder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ import { verifyAttestationToken } from "../utils/helpers";
describe("[AAD] Attestation Client", function() {
let recorder: Recorder;

beforeEach(function() {
// eslint-disable-next-line no-invalid-this
beforeEach(/** @this Mocha.Context */ function() {
recorder = createRecorder(this);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ import { verifyAttestationToken } from "../utils/helpers";
describe("PolicyGetSetTests ", function() {
let recorder: Recorder;

beforeEach(function() {
// eslint-disable-next-line no-invalid-this
beforeEach(/** @this Mocha.Context */ function() {
recorder = createRecorder(this);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import { verifyAttestationToken } from "../utils/helpers";
describe("PolicyManagementTests ", function() {
let recorder: Recorder;

beforeEach(function() {
// eslint-disable-next-line no-invalid-this
beforeEach(/** @this Mocha.Context */ function() {
recorder = createRecorder(this);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ import { Buffer } from "../utils/Buffer";
describe("TokenCertTests", function() {
let recorder: Recorder;

beforeEach(function() {
// eslint-disable-next-line no-invalid-this
beforeEach(/** @this Mocha.Context */ function() {
recorder = createRecorder(this);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,35 +50,36 @@ describe.skip("ContainerRegistryClient functional tests", function() {
// NOTE: use of "function" and not ES6 arrow-style functions with the
// beforeEach hook is IMPORTANT due to the use of `this` in the function
// body.
beforeEach(function(this: Context) {
// The recorder has some convenience methods, and we need to store a
// reference to it so that we can `stop()` the recorder later in the
// `afterEach` hook.
// eslint-disable-next-line no-invalid-this
recorder = record(this, {
// == Recorder Environment Setup == Add the replaceable variables from
// above
replaceableVariables,
beforeEach(
/** @this Mocha.Context */ function(this: Context) {
// The recorder has some convenience methods, and we need to store a
// reference to it so that we can `stop()` the recorder later in the
// `afterEach` hook.
recorder = record(this, {
// == Recorder Environment Setup == Add the replaceable variables from
// above
replaceableVariables,

// We don't use this in the template, but if we had any query parameters
// we wished to discard, we could add them here
queryParametersToSkip: [],
// We don't use this in the template, but if we had any query parameters
// we wished to discard, we could add them here
queryParametersToSkip: [],

// Finally, we need to remove the AAD `access_token` from any requests.
// This is very important, as it cannot be removed using environment
// variable or query parameter replacement. The
// `customizationsOnRecordings` field allows us to make arbitrary
// replacements within recordings.
customizationsOnRecordings: [
(recording: any): any =>
recording.replace(/"access_token":"[^"]*"/g, `"access_token":"access_token"`)
]
});
// Finally, we need to remove the AAD `access_token` from any requests.
// This is very important, as it cannot be removed using environment
// variable or query parameter replacement. The
// `customizationsOnRecordings` field allows us to make arbitrary
// replacements within recordings.
customizationsOnRecordings: [
(recording: any): any =>
recording.replace(/"access_token":"[^"]*"/g, `"access_token":"access_token"`)
]
});

// We'll be able to refer to the instantiated `client` in tests, since we
// initialize it before each test
client = createTestClient();
});
// We'll be able to refer to the instantiated `client` in tests, since we
// initialize it before each test
client = createTestClient();
}
);

// After each test, we need to stop the recording.
afterEach(async function() {
Expand Down
4 changes: 4 additions & 0 deletions sdk/containerregistry/container-registry/tsdoc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"$schema": "https://developer.microsoft.com/json-schemas/tsdoc/v0/tsdoc.schema.json",
"extends": ["../../../tsdoc.json"]
}
3 changes: 1 addition & 2 deletions sdk/core/core-http/test/defaultHttpClientTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,8 @@ describe("defaultHttpClient", function() {
}
});

it("should give a graceful error for nonexistent hosts", async function() {
it("should give a graceful error for nonexistent hosts", /** @this Mocha.Context */ async function() {
// Increase timeout to give the request time to fail
// eslint-disable-next-line no-invalid-this
this.timeout(10000);
const requestUrl = "http://fake.domain";
const request = new WebResource(requestUrl, "GET");
Expand Down
4 changes: 3 additions & 1 deletion sdk/core/core-lro/test/utils/testOperation.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
/* eslint-disable no-invalid-this */

import { HttpOperationResponse, RequestOptionsBase } from "@azure/core-http";
import { AbortSignalLike } from "@azure/abort-controller";
Expand All @@ -22,6 +21,7 @@ export interface TestOperationState extends PollOperationState<string> {

export interface TestOperation extends PollOperation<TestOperationState, string> {}

/** @this TestOperation */
async function update(
this: TestOperation,
options: {
Expand Down Expand Up @@ -70,6 +70,7 @@ async function update(
return makeOperation(newState);
}

/** @this TestOperation */
async function cancel(
this: TestOperation,
options: { abortSignal?: AbortSignal } = {}
Expand Down Expand Up @@ -100,6 +101,7 @@ async function cancel(
});
}

/** @this TestOperation */
function toString(this: TestOperation): string {
return JSON.stringify({
state: this.state
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/logger/src/debug.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/* eslint-disable no-invalid-this */

import { log } from "./log";

/**
Expand Down Expand Up @@ -157,6 +155,7 @@ function createDebugger(namespace: string): Debugger {
return newDebugger;
}

/** @this Debugger */
function destroy(this: Debugger): boolean {
const index = debuggers.indexOf(this);
if (index >= 0) {
Expand All @@ -166,6 +165,7 @@ function destroy(this: Debugger): boolean {
return false;
}

/** @this Debugger */
function extend(this: Debugger, namespace: string): Debugger {
const newDebugger = createDebugger(`${this.namespace}:${namespace}`);
newDebugger.log = this.log;
Expand Down
1 change: 0 additions & 1 deletion sdk/cosmosdb/cosmos/test/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"extends": ["../.eslintrc.json"],
"rules": {
"no-console": "off",
"no-invalid-this": "off",
"space-before-function-paren": "off"
}
}
4 changes: 2 additions & 2 deletions sdk/cosmosdb/cosmos/test/internal/session.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function getCollection2TokenMap(
return (sessionContainer as any).collectionResourceIdToSessionTokens;
}

describe("Session Token", function() {
describe("Session Token", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 20000);

const containerId = "sessionTestColl";
Expand Down Expand Up @@ -304,7 +304,7 @@ describe("Session Token", function() {
spy.restore();
});

it("validate 'lsn not caught up' error for higher lsn and clearing session token", async function() {
it("validate 'lsn not caught up' error for higher lsn and clearing session token", /** @this Mocha.Context */ async function() {
this.retries(2);
const database = await getTestDatabase("session test", client);

Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/test/internal/unit/auth.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { getAuthorizationTokenUsingResourceTokens } from "../../../src/auth";
import assert from "assert";

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);

it("should find exact match", async function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
removeAllDatabases
} from "../common/TestHelpers";

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/test/public/functional/client.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import AbortController from "node-abort-controller";
import { UsernamePasswordCredential } from "@azure/identity";

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 20000);

describe("Validate client request timeout", function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import assert from "assert";
import { removeAllDatabases, getTestContainer } from "../common/TestHelpers";

describe("Conflicts", function() {
describe("Conflicts", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
import { SpatialType } from "../../../src";
import { GeospatialType } from "../../../src";

describe("Containers", function() {
describe("Containers", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { DatabaseRequest } from "../../../src";

const client = new CosmosClient({ endpoint, key: masterKey });

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import { endpoint, masterKey } from "../common/_testConfig";

const client = new CosmosClient({ endpoint, key: masterKey });

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
});
beforeEach(
/** @this Mocha.Context */ async function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
}
);

describe("validate database account functionality", function() {
it("nativeApi Should get database account successfully name based", async function() {
Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/test/public/functional/item.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ interface TestItem {
replace?: string;
}

describe("Item CRUD", function() {
describe("Item CRUD", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
12 changes: 7 additions & 5 deletions sdk/cosmosdb/cosmos/test/public/functional/offer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@ const validateOfferResponseBody = function(offer: any): void {
assert(offer._self.indexOf(offer.id) !== -1, "Offer id not contained in offer self link.");
};

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);

beforeEach(async function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
await removeAllDatabases();
});
beforeEach(
/** @this Mocha.Context */ async function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
await removeAllDatabases();
}
);

describe("Validate Offer CRUD", function() {
it("nativeApi Should do offer read and query operations successfully name based single partition collection", async function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
replaceOrUpsertPermission
} from "../common/TestHelpers";

describe("NodeJS CRUD Tests", function() {
describe("NodeJS CRUD Tests", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
6 changes: 3 additions & 3 deletions sdk/cosmosdb/cosmos/test/public/functional/query.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ if (!Symbol || !Symbol.asyncIterator) {
(Symbol as any).asyncIterator = Symbol.for("Symbol.asyncIterator");
}

describe("Queries", function() {
describe("Queries", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
before(async function() {
await removeAllDatabases();
Expand Down Expand Up @@ -47,7 +47,7 @@ describe("Queries", function() {
});
});

describe("QueryIterator", function() {
describe("QueryIterator", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 30000);
let resources: { container: Container; doc1: any; doc2: any; doc3: any };

Expand Down Expand Up @@ -143,7 +143,7 @@ describe("Queries", function() {
}
});

describe("SUM query iterator", function() {
describe("SUM query iterator", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 30000);

it("returns undefined sum with null value in aggregator", async function() {
Expand Down
2 changes: 1 addition & 1 deletion sdk/cosmosdb/cosmos/test/public/functional/spatial.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import assert from "assert";
import { Database, DataType, IndexKind } from "../../../src";
import { createOrUpsertItem, getTestDatabase, removeAllDatabases } from "../common/TestHelpers";

describe("Spatial Indexes", function() {
describe("Spatial Indexes", /** @this Mocha.Context */ function() {
this.timeout(process.env.MOCHA_TIMEOUT || 10000);
beforeEach(async function() {
await removeAllDatabases();
Expand Down
Loading

0 comments on commit 0c3f72f

Please sign in to comment.