Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Sep 26, 2024

Problem

continuation of reliability work

Solution

tryInstallLsp in src/amazonq/lsp/lspController.ts captures many potentially high risk paths in a single function, such as some I/O operations, and heavy use of the admZip dependency.

Two performance tests were added, one for handling zipping and maniupulating many small files, and one with few large files. Thresholds are set very leniently to avoid flaky tests.

The "many files" test does 250 files w/ 10 bytes each. The "large files" does 10 files w/ 1000 bytes each. These values were chosen to avoid flakiness in CI.

Included in this PR is removing the use of fs-extra in src/amazonq/lsp/lspController.ts

@Hweinstock Hweinstock changed the title tests(amazonq): performance test for LSP setup. test(amazonq): performance test for LSP setup. Sep 26, 2024
@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

@Hweinstock Hweinstock marked this pull request as ready for review October 2, 2024 14:21
@Hweinstock Hweinstock requested review from a team as code owners October 2, 2024 14:21
Copy link
Contributor

@jpinkney-aws jpinkney-aws left a comment

Choose a reason for hiding this comment

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

Since cpu/system are non deterministic we should document our performance testing strategy somewhere i.g. cpu/memory are purposely set higher to catch instances where cpu usage/memory/duration explodes on a code change and then use fs calls for determinism

@Hweinstock Hweinstock merged commit 401fc1e into aws:master Oct 7, 2024
21 of 24 checks passed
@Hweinstock Hweinstock deleted the pTests/tryInstallLsp branch October 7, 2024 16:08
Copy link
Contributor

@justinmk3 justinmk3 Oct 7, 2024

Choose a reason for hiding this comment

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

probably should start a subdir in testInteg, maybe testInteg/perf/ . Or perf tests could in "normal" places (testInteg/path/to/module/) but use a foo.perf.test.ts filename convention.

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.

3 participants