Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(test): fix flaky test related to ec2.updateStatus #5758

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Oct 9, 2024

Problem

Follow up to: #5698
Error: #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.

Copy link

github-actions bot commented Oct 9, 2024

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

@Hweinstock Hweinstock marked this pull request as ready for review October 9, 2024 21:38
@Hweinstock Hweinstock requested a review from a team as a code owner October 9, 2024 21:38
@Hweinstock Hweinstock changed the title fix(test): fix flaky test related to ec2.updateStatus (WIP) fix(test): fix flaky test related to ec2.updateStatus Oct 9, 2024
@Hweinstock Hweinstock marked this pull request as draft October 9, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant