From 2efb7a231bc79a064cf5c7c498351a90de184f0d Mon Sep 17 00:00:00 2001 From: Hweinstock <42325418+Hweinstock@users.noreply.github.com> Date: Mon, 14 Oct 2024 16:03:29 -0400 Subject: [PATCH] fix(test): fix flaky test related to ec2.updateStatus (#5758) ## Problem Follow up to: https://github.com/aws/aws-toolkit-vscode/pull/5698 Fix: https://github.com/aws/aws-toolkit-vscode/issues/5750 ### First Problem Identified The current tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts do not clear the `PollingSet` in between tests, therefore the timer continues to run. This not only results in inconsistent state between the items in the `PollingSet` and the items stored on the parent node itself, but also allows the `PollingSet` action to trigger unexpectedly in the middle of another test. When this happens, and the states don't match, the `instanceId` from the `PollingSet` could potentially not be found on the parent node, resulting in an undefined node. Then, calling `updateStatus` on this node could explain the error. For this error to happen, the `PollingSet` timer must trigger at a very specific point (in the middle of another test), making it difficult to debug and reproduce, and causing occasional flaky test failures. ### Second Problem Identified The tests in https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts setup an unnatural state. The `testNode` sets its parent to the `testParentNode`, but `testParentNode` is unaware this child exists. This is problematic because when the child reports its status as pending to the parent, the parent will throw an error since it can't find the child. (example: https://d1ihu6zq92vp9p.cloudfront.net/7b1e96c7-f2b7-4a8e-927c-b7aa84437960/report.html) This type of state should not be allowed. ## Solution ### Solution to first problem - clear the set, and the timer in-between each test. ### Solution to second problem - make this confusing state impossible by adding the child to the parent and parent to the child together. - stub the resulting API call its makes. - Disable the polling set timer in the second tests (via sinon stubbing) since it isn't relevant to what's being tested. ### Tangential Work included in PR: - Throw our own error when an `instanceId` is not in map on the parentNode. This will make it easier to catch if something goes wrong. - Clean up test file: `src/test/awsService/ec2/explorer/ec2ParentNode.test.ts`. --- License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --- .../ec2/explorer/ec2InstanceNode.ts | 3 +- .../awsService/ec2/explorer/ec2ParentNode.ts | 22 ++++- .../ec2/explorer/ec2InstanceNode.test.ts | 8 +- .../ec2/explorer/ec2ParentNode.test.ts | 87 ++++++++++++------- 4 files changed, 85 insertions(+), 35 deletions(-) diff --git a/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts b/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts index ded19d5059a..7078b6bdb02 100644 --- a/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts +++ b/packages/core/src/awsService/ec2/explorer/ec2InstanceNode.ts @@ -29,6 +29,7 @@ export class Ec2InstanceNode extends AWSTreeNodeBase implements AWSResourceNode public readonly instance: SafeEc2Instance ) { super('') + this.parent.addChild(this) this.updateInstance(instance) this.id = this.InstanceId } @@ -41,7 +42,7 @@ export class Ec2InstanceNode extends AWSTreeNodeBase implements AWSResourceNode this.tooltip = `${this.name}\n${this.InstanceId}\n${this.instance.LastSeenStatus}\n${this.arn}` if (this.isPending()) { - this.parent.pollingSet.start(this.InstanceId) + this.parent.trackPendingNode(this.InstanceId) } } diff --git a/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts b/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts index f751f7e0c61..854e6eacd1c 100644 --- a/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts +++ b/packages/core/src/awsService/ec2/explorer/ec2ParentNode.ts @@ -41,6 +41,13 @@ export class Ec2ParentNode extends AWSTreeNodeBase { }) } + public trackPendingNode(instanceId: string) { + if (!this.ec2InstanceNodes.has(instanceId)) { + throw new Error(`Attempt to track ec2 node ${instanceId} that isn't a child`) + } + this.pollingSet.start(instanceId) + } + public async updateChildren(): Promise { const ec2Instances = await (await this.ec2Client.getInstances()).toMap((instance) => instance.InstanceId) updateInPlace( @@ -52,9 +59,18 @@ export class Ec2ParentNode extends AWSTreeNodeBase { ) } + public getInstanceNode(instanceId: string): Ec2InstanceNode { + const childNode = this.ec2InstanceNodes.get(instanceId) + if (childNode) { + return childNode + } else { + throw new Error(`Node with id ${instanceId} from polling set not found`) + } + } + private async updatePendingNodes() { for (const instanceId of this.pollingSet.values()) { - const childNode = this.ec2InstanceNodes.get(instanceId)! + const childNode = this.getInstanceNode(instanceId) await this.updatePendingNode(childNode) } } @@ -71,6 +87,10 @@ export class Ec2ParentNode extends AWSTreeNodeBase { this.ec2InstanceNodes = new Map() } + public addChild(node: Ec2InstanceNode) { + this.ec2InstanceNodes.set(node.InstanceId, node) + } + public async refreshNode(): Promise { await this.clearChildren() await vscode.commands.executeCommand('aws.refreshAwsExplorerNode', this) diff --git a/packages/core/src/test/awsService/ec2/explorer/ec2InstanceNode.test.ts b/packages/core/src/test/awsService/ec2/explorer/ec2InstanceNode.test.ts index a5976ef8f01..5299d2a080d 100644 --- a/packages/core/src/test/awsService/ec2/explorer/ec2InstanceNode.test.ts +++ b/packages/core/src/test/awsService/ec2/explorer/ec2InstanceNode.test.ts @@ -12,6 +12,8 @@ import { } from '../../../../awsService/ec2/explorer/ec2InstanceNode' import { Ec2Client, SafeEc2Instance, getNameOfInstance } from '../../../../shared/clients/ec2Client' import { Ec2ParentNode } from '../../../../awsService/ec2/explorer/ec2ParentNode' +import * as sinon from 'sinon' +import { PollingSet } from '../../../../shared/utilities/pollingSet' describe('ec2InstanceNode', function () { let testNode: Ec2InstanceNode @@ -30,12 +32,16 @@ describe('ec2InstanceNode', function () { ], LastSeenStatus: 'running', } + sinon.stub(Ec2InstanceNode.prototype, 'updateStatus') + // Don't want to be polling here, that is tested in ../ec2ParentNode.test.ts + // disabled here for convenience (avoiding race conditions with timeout) + sinon.stub(PollingSet.prototype, 'start') const testClient = new Ec2Client('') const testParentNode = new Ec2ParentNode(testRegion, testPartition, testClient) testNode = new Ec2InstanceNode(testParentNode, testClient, 'testRegion', 'testPartition', testInstance) }) - this.beforeEach(function () { + beforeEach(function () { testNode.updateInstance(testInstance) }) diff --git a/packages/core/src/test/awsService/ec2/explorer/ec2ParentNode.test.ts b/packages/core/src/test/awsService/ec2/explorer/ec2ParentNode.test.ts index cb25c9f885c..788c97db046 100644 --- a/packages/core/src/test/awsService/ec2/explorer/ec2ParentNode.test.ts +++ b/packages/core/src/test/awsService/ec2/explorer/ec2ParentNode.test.ts @@ -17,17 +17,14 @@ import { EC2 } from 'aws-sdk' import { AsyncCollection } from '../../../../shared/utilities/asyncCollection' import * as FakeTimers from '@sinonjs/fake-timers' import { installFakeClock } from '../../../testUtil' -import { PollingSet } from '../../../../shared/utilities/pollingSet' describe('ec2ParentNode', function () { let testNode: Ec2ParentNode - let defaultInstances: SafeEc2Instance[] let client: Ec2Client let getInstanceStub: sinon.SinonStub<[filters?: EC2.Filter[] | undefined], Promise>> let clock: FakeTimers.InstalledClock let refreshStub: sinon.SinonStub<[], Promise> - let clearTimerStub: sinon.SinonStub<[], void> - + let statusUpdateStub: sinon.SinonStub<[status: string], Promise> const testRegion = 'testRegion' const testPartition = 'testPartition' @@ -45,36 +42,19 @@ describe('ec2ParentNode', function () { client = new Ec2Client(testRegion) clock = installFakeClock() refreshStub = sinon.stub(Ec2InstanceNode.prototype, 'refreshNode') - clearTimerStub = sinon.stub(PollingSet.prototype, 'clearTimer') - defaultInstances = [ - { Name: 'firstOne', InstanceId: '0', LastSeenStatus: 'running' }, - { Name: 'secondOne', InstanceId: '1', LastSeenStatus: 'running' }, - ] + statusUpdateStub = sinon.stub(Ec2Client.prototype, 'getInstanceStatus') }) beforeEach(function () { getInstanceStub = sinon.stub(Ec2Client.prototype, 'getInstances') - defaultInstances = [ - { Name: 'firstOne', InstanceId: '0', LastSeenStatus: 'running' }, - { Name: 'secondOne', InstanceId: '1', LastSeenStatus: 'stopped' }, - ] - - getInstanceStub.callsFake(async () => - intoCollection( - defaultInstances.map((instance) => ({ - InstanceId: instance.InstanceId, - Tags: [{ Key: 'Name', Value: instance.Name }], - })) - ) - ) - testNode = new Ec2ParentNode(testRegion, testPartition, client) refreshStub.resetHistory() - clearTimerStub.resetHistory() }) afterEach(function () { getInstanceStub.restore() + testNode.pollingSet.clear() + testNode.pollingSet.clearTimer() }) after(function () { @@ -91,10 +71,14 @@ describe('ec2ParentNode', function () { }) it('has instance child nodes', async function () { - getInstanceStub.resolves(mapToInstanceCollection(defaultInstances)) + const instances = [ + { Name: 'firstOne', InstanceId: '0', LastSeenStatus: 'running' }, + { Name: 'secondOne', InstanceId: '1', LastSeenStatus: 'stopped' }, + ] + getInstanceStub.resolves(mapToInstanceCollection(instances)) const childNodes = await testNode.getChildren() - assert.strictEqual(childNodes.length, defaultInstances.length, 'Unexpected child count') + assert.strictEqual(childNodes.length, instances.length, 'Unexpected child count') childNodes.forEach((node) => assert.ok(node instanceof Ec2InstanceNode, 'Expected child node to be Ec2InstanceNode') @@ -151,14 +135,13 @@ describe('ec2ParentNode', function () { ] getInstanceStub.resolves(mapToInstanceCollection(instances)) - await testNode.updateChildren() assert.strictEqual(testNode.pollingSet.size, 1) getInstanceStub.restore() }) it('does not refresh explorer when timer goes off if status unchanged', async function () { - const statusUpdateStub = sinon.stub(Ec2Client.prototype, 'getInstanceStatus').resolves('pending') + statusUpdateStub = statusUpdateStub.resolves('pending') const instances = [ { Name: 'firstOne', InstanceId: '0', LastSeenStatus: 'pending' }, { Name: 'secondOne', InstanceId: '1', LastSeenStatus: 'stopped' }, @@ -170,16 +153,56 @@ describe('ec2ParentNode', function () { await testNode.updateChildren() await clock.tickAsync(6000) sinon.assert.notCalled(refreshStub) - statusUpdateStub.restore() getInstanceStub.restore() }) it('does refresh explorer when timer goes and status changed', async function () { + statusUpdateStub = statusUpdateStub.resolves('running') + const instances = [{ Name: 'firstOne', InstanceId: '0', LastSeenStatus: 'pending' }] + + getInstanceStub.resolves(mapToInstanceCollection(instances)) + await testNode.updateChildren() + sinon.assert.notCalled(refreshStub) - const statusUpdateStub = sinon.stub(Ec2Client.prototype, 'getInstanceStatus').resolves('running') - testNode.pollingSet.add('0') await clock.tickAsync(6000) sinon.assert.called(refreshStub) - statusUpdateStub.restore() + }) + + it('returns the node when in the map', async function () { + const instances = [{ Name: 'firstOne', InstanceId: 'node1', LastSeenStatus: 'pending' }] + + getInstanceStub.resolves(mapToInstanceCollection(instances)) + await testNode.updateChildren() + const node = testNode.getInstanceNode('node1') + assert.strictEqual(node.InstanceId, instances[0].InstanceId) + getInstanceStub.restore() + }) + + it('throws error when node not in map', async function () { + const instances = [{ Name: 'firstOne', InstanceId: 'node1', LastSeenStatus: 'pending' }] + + getInstanceStub.resolves(mapToInstanceCollection(instances)) + await testNode.updateChildren() + assert.throws(() => testNode.getInstanceNode('node2')) + getInstanceStub.restore() + }) + + it('adds node to polling set when asked to track it', async function () { + const instances = [{ Name: 'firstOne', InstanceId: 'node1', LastSeenStatus: 'pending' }] + + getInstanceStub.resolves(mapToInstanceCollection(instances)) + await testNode.updateChildren() + testNode.trackPendingNode('node1') + assert.strictEqual(testNode.pollingSet.size, 1) + getInstanceStub.restore() + }) + + it('throws error when asked to track non-child node', async function () { + const instances = [{ Name: 'firstOne', InstanceId: 'node1', LastSeenStatus: 'pending' }] + + getInstanceStub.resolves(mapToInstanceCollection(instances)) + await testNode.updateChildren() + assert.throws(() => testNode.trackPendingNode('node2')) + getInstanceStub.restore() }) })