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

Try to stabilize tests in CI #1128

Merged
merged 1 commit into from
Feb 23, 2025
Merged

Try to stabilize tests in CI #1128

merged 1 commit into from
Feb 23, 2025

Conversation

cte
Copy link
Collaborator

@cte cte commented Feb 23, 2025

Description

  • Update the checkpoint services tests to use better tmp directory isolation
  • Increase timeout for applyGitFallback tests
  • Separate extensions and webview tests in CI
  • Update jest and ts-jest in webview-ui

I used this to debug the flakey test on the CI machine, but it seemed to run fine in isolation.

I first tried increasing the V8 heap size, but that just allowed the tests to run long enough to hit some jest timeouts.

Next I tried logging the performance of the shadow checkpointer tests, and I found that simply disabling the dot-reporter and enabling console.log calls seemed to fix things. That also turned out to be a red herring.

Digging a little bit deeper, it seems like there's an interaction between the shadow checkpointer and the applyGitFallback function, which isn't surprising because they both create git repositories, and one might end up creating a nested git repo which could cause problems. Increasing the timeout on the applyGitFallback tests seem to mitigate the issue, but we'll see if this continues to be flakey with future CI runs. As a follow-up, I will write additional tests to validate that the shadow checkpoint correctly handles nested git repos. It's possible that the tests environment is problematic b/c we are mocking globby.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the patterns of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Additional context

Related Issues

Reviewers


Important

Stabilize CI tests by improving isolation, increasing timeouts, separating jobs, and cleaning logs.

  • CI Workflow:
    • Separate test-extension and test-webview jobs in .github/workflows/code-qa.yml.
    • Use npx jest --silent instead of jest-simple-dot-reporter for cleaner logs.
  • Tests:
    • Increase timeout for applyGitFallback tests in edit-strategies.test.ts.
    • Use isolated tmp directories in LocalCheckpointService.test.ts and ShadowCheckpointService.test.ts.
  • Package Management:
    • Remove jest-simple-dot-reporter from package.json and webview-ui/package.json.

This description was created by Ellipsis for 2e6bda9. It will automatically update as commits are pushed.

Copy link

changeset-bot bot commented Feb 23, 2025

⚠️ No Changeset found

Latest commit: f11c0af

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 23, 2025
@cte cte force-pushed the cte/stabilize-tests-in-ci branch from c4b5980 to 42c419e Compare February 23, 2025 08:11
working-directory: webview-ui
run: npx jest

unit-test:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a branch rule dictating that "unit-test" must pass in order for the PR to be merged, hence this composite job.

@cte cte force-pushed the cte/stabilize-tests-in-ci branch 3 times, most recently from 02c54e4 to bbd8a96 Compare February 23, 2025 08:36
"jest": "^27.5.1",
"jest-environment-jsdom": "^27.5.1",
"jest-simple-dot-reporter": "^1.0.5",
"jest": "^29.7.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well match the jest and ts-jest versions that we use to test the extension.

@cte cte force-pushed the cte/stabilize-tests-in-ci branch from 18cdf84 to 9b734c5 Compare February 23, 2025 09:12
@cte cte changed the title Stabilize tests in CI Try to stabilize tests in CI Feb 23, 2025
@@ -1,3 +1,5 @@
/// <reference types="jest" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since @types/mocha and @types/jest both define expect, beforeEach, afterEach, etc we need to indicate which version of these global functions are being used. I'm not sure what the "correct" fix for this is, but it's unfortunate that we have this conflict.

Mocha is only needed for integration tests, and it seems like that could live in a separate package (kind of like webview-ui).

@cte cte force-pushed the cte/stabilize-tests-in-ci branch from 2e6bda9 to f11c0af Compare February 23, 2025 10:28
@@ -1,3 +1,4 @@
{
"extends": "react-app"
"extends": "react-app",
"ignorePatterns": ["!.storybook"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -275,7 +277,7 @@ describe("applyGitFallback", () => {
expect(result.result.join("\n")).toEqual("line1\nnew line2\nline3")
expect(result.confidence).toBe(1)
expect(result.strategy).toBe("git-fallback")
})
}, 10_000)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this implies that whatever we’re doing with git in the experimental diff algo is taking between 5 and 10 seconds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encountered this timeout while debugging, and increasing it seemed to suddenly make the problems go away. Undoing this change seemed to make the problem reliably come back. One theory I had was that it impacted the order or sequencing of the tests such that the failure stopped happening. I added the --randomize flag to jest to test that theory but we get a bunch of other failures since some of our tests do seem to rely on being run in a particular order (which we should fix).

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 23, 2025
@cte cte merged commit e413a59 into main Feb 23, 2025
11 checks passed
@cte cte deleted the cte/stabilize-tests-in-ci branch February 23, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants