refactor: re-arrange some globals and add `clock` #2343

JadenSimon · 2021-11-29T21:19:47Z

Problem

The insiders version of VS Code broke our tests due to our reliance on 'fake timers'. The library itself isn't bad, it's just we're stubbing the global scheduling functions.

Solution

Add a 'clock' object to our extension globals. This should be used in 'production' code while test code may use either the 'real' functions or the scoped-down version depending on the use case. Also refactored the globals code a bit to be more ergonomic by removing the namespace and using pure modules. Some work still needs to be done to make the initialization cleaner which should help a lot with making tests cleaner and easier to write.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

JadenSimon · 2021-11-29T21:22:10Z

src/test/shared/utilities/timeoutUtils.test.ts

@@ -99,29 +98,42 @@ describe('timeoutUtils', async function () {
        })

        it('Correctly reports elapsed time with refresh', async function () {
-            const longTimer = new timeoutUtils.Timeout(10)
-            clock.tick(5)
+            // TODO: fake timers `refresh` is not implemented correctly for the non-global case


I found a bug with fake timers that I'll need to upstream a fix for. It only happens when using a different global object to fake.

justinmk3 · 2021-11-30T18:34:59Z

src/apprunner/explorer/apprunnerNode.ts

            this.pollTimer = undefined
        }
    }

    public startPolling(id: string): void {
        this.pollingNodes.add(id)
-        this.pollTimer = this.pollTimer ?? setInterval(this.refresh.bind(this), POLLING_INTERVAL)
+        this.pollTimer = this.pollTimer ?? awsToolkit.clock.setInterval(this.refresh.bind(this), POLLING_INTERVAL)


should we have a lint rule to prevent direct usage of the window.setInterval / setTimeout / etc?

I don't think it should be required, at least not yet. It boils down to where if you want to test your code with the fake clock then you must use the toolkit-owned clock. But using the real thing has no effect if you are not mocking the clock.

I'll still dig up the linting PR so we can at least have a means to enforce some of these things through code. But I consider it relatively low priority.

But using the real thing has no effect if you are not mocking the clock.

Main concern for me is if someone mocks the clock but doesn't use this global wrapper, then has weird effects and doesn't know why.

But I consider it relatively low priority.

Not a blocker here. Backlogged: #2347

Main concern for me is if someone mocks the clock but doesn't use this global wrapper, then has weird effects and doesn't know why.

Yup that's the biggest concern. I'll add a big warning for installFakeClock for now until linting can be added.

src/integrationTest/integrationTestsUtilities.ts

justinmk3 · 2021-11-30T18:37:51Z

src/lambda/commands/deleteLambda.ts

@@ -40,7 +40,7 @@ export async function deleteLambda({
    if (!deleteParams.functionName) {
        return
    }
-    const startTime = new Date()
+    const startTime = new awsToolkit.clock.Date()


lint rule for this?

JadenSimon · 2021-11-30T19:21:25Z

src/shared/extensionGlobals.ts

 /**
 * Namespace for common variables used globally in the extension.
 * All variables here must be initialized in the activate() method of extension.ts
 */
-export namespace ext {


I removed the namespace since namespaces are mostly considered 'legacy', at least for Node applications. Best just to use ES6 module syntax directly.

Thanks. This has been on our TODO list for awhile :)

JadenSimon · 2021-11-30T19:33:37Z

src/stepFunctions/asl/aslServer.ts

@@ -97,56 +96,54 @@ let resultLimit = Number.MAX_VALUE

 // After the server has started the client sends an initialize request. The server receives
 // in the passed params the rootPath of the workspace plus the client capabilities.
-connection.onInitialize(


I think this file hadn't been run with prettier so it got moved around quite a bit.

JadenSimon · 2021-11-30T19:35:11Z

src/stepFunctions/activation.ts

-    ext.visualizationResourcePaths.stateMachineCustomThemeCSS = vscode.Uri.file(
-        context.asAbsolutePath(join('media', 'css', 'stateMachineRender.css'))
-    )
+    return {


For the most part we shouldn't assign directly to the global object. Just looking at the code that uses ext I can tell there's tons of potential avenues for bugs during testing because we assign in two places.

JadenSimon · 2021-11-30T19:38:27Z

src/apigateway/explorer/apiGatewayNodes.ts

@@ -16,6 +16,7 @@ import { ApiGatewayClient } from '../../shared/clients/apiGatewayClient'
 import { RestApi } from 'aws-sdk/clients/apigateway'
 import { toArrayAsync, toMap, updateInPlace } from '../../shared/utilities/collectionUtils'
 import { RestApiNode } from './apiNodes'
+import globals from '../../shared/extensionGlobals'


When I changed the import it ended up getting placed at the bottom. All well I guess. I wonder if we can have prettier sort the imports or something.

JadenSimon · 2021-12-01T02:02:00Z

Apparently describe blocks are executed on module load in the minimum VS Code version. Really not a big fan of having two separate Date references. Need to either add a tiny bit of abstraction to the scoped clock or figure out a different way to separate them.

JadenSimon · 2021-12-01T02:02:23Z

/runintegrationtests ...

JadenSimon added 2 commits November 29, 2021 09:11

Fix tests

870f48c

Move global stuff around

7dbede2

JadenSimon requested a review from a team as a code owner November 29, 2021 21:19

JadenSimon commented Nov 29, 2021

View reviewed changes

ext -> awsToolkit

f16f0e4

justinmk3 reviewed Nov 30, 2021

View reviewed changes

src/integrationTest/integrationTestsUtilities.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Nov 30, 2021

View reviewed changes

JadenSimon added 2 commits November 30, 2021 10:57

Rename to globals

2968239

Not needed here

b3cd3c4

JadenSimon commented Nov 30, 2021

View reviewed changes

justinmk3 approved these changes Nov 30, 2021

View reviewed changes

justinmk3 mentioned this pull request Nov 30, 2021

lint: custom lint rules #2347

Open

JadenSimon added 7 commits November 30, 2021 16:47

Clean up

23172c0

Add log

be49a05

Fix

1d0bfdd

Fix?

fa21c8c

Remove scoped Date from diskCache test

3fff9a3

Fix some tests

b9cd848

Minimum

987f684

JadenSimon added 3 commits December 1, 2021 09:27

Fix integ

996249b

Merge

831d602

Flakey test

2e62081

JadenSimon merged commit 6f1eabb into master Dec 1, 2021

JadenSimon deleted the sijaden/clocks branch December 1, 2021 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: re-arrange some globals and add `clock` #2343

refactor: re-arrange some globals and add `clock` #2343

JadenSimon commented Nov 29, 2021 •

edited

Loading

JadenSimon Nov 29, 2021

justinmk3 Nov 30, 2021

JadenSimon Nov 30, 2021 •

edited

Loading

justinmk3 Nov 30, 2021

JadenSimon Dec 1, 2021

justinmk3 Nov 30, 2021

JadenSimon Nov 30, 2021

justinmk3 Nov 30, 2021

JadenSimon Nov 30, 2021

JadenSimon Nov 30, 2021

JadenSimon Nov 30, 2021

JadenSimon commented Dec 1, 2021

JadenSimon commented Dec 1, 2021 •

edited

Loading

refactor: re-arrange some globals and add clock #2343

refactor: re-arrange some globals and add clock #2343

Conversation

JadenSimon commented Nov 29, 2021 • edited Loading

Problem

Solution

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JadenSimon Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JadenSimon commented Dec 1, 2021

JadenSimon commented Dec 1, 2021 • edited Loading

refactor: re-arrange some globals and add `clock` #2343

refactor: re-arrange some globals and add `clock` #2343

JadenSimon commented Nov 29, 2021 •

edited

Loading

JadenSimon Nov 30, 2021 •

edited

Loading

JadenSimon commented Dec 1, 2021 •

edited

Loading