Skip to content

Conversation

@Hweinstock
Copy link
Contributor

Problem

Performance test involve running the same code 10 times, and often its demanding code. This should live with integ rather than unit tests to avoid slow down.

Solution


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

@Hweinstock Hweinstock marked this pull request as ready for review October 7, 2024 18:04
@Hweinstock Hweinstock requested a review from a team as a code owner October 7, 2024 18:04
@justinmk3
Copy link
Contributor

the integ test failures appear to be unrelated since they are also failing on master.

1) tryInstallLsp
       performance tests
         many small files in zip performance tests
           "after all" hook for "many small files in zip - test run 10":

      AssertionError [ERR_ASSERTION]: Expected total user CPU usage for many small files in zip to be less than 100. Actual user CPU usage was 108.90424026793211
      + expected - actual

      -false
      +true
      
      at assertPerformanceMetrics (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/shared/performance/performance.ts:182:11)
      at Context.<anonymous> (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/shared/performance/performance.ts:160:13)
      at Context.fn (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/test/setupUtil.ts:33:24)
      at processImmediate (node:internal/timers:483:21)
      at process.topLevelDomainCallback (node:domain:160:15)
      at process.callbackTrampoline (node:internal/async_hooks:128:24)

  2) startSecurityScanPerformanceTest
       Should calculate cpu and memory usage for file scans performance tests
         "after all" hook for "Should calculate cpu and memory usage for file scans - test run 10":

      AssertionError [ERR_ASSERTION]: Expected total user CPU usage for Should calculate cpu and memory usage for file scans to be less than 50. Actual user CPU usage was 98.38089502683101
      + expected - actual

      -false
      +true
      
      at assertPerformanceMetrics (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/shared/performance/performance.ts:182:11)
      at Context.<anonymous> (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/shared/performance/performance.ts:160:13)
      at Context.fn (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/test/setupUtil.ts:33:24)
      at processImmediate (node:internal/timers:483:21)
      at process.topLevelDomainCallback (node:domain:160:15)
      at process.callbackTrampoline (node:internal/async_hooks:128:24)

  3) collectFiles
       returns all files in the workspace not excluded by gitignore:

      AssertionError [ERR_ASSERTION]: telemetry metric 1 (of 1) not found (in the expected order): "function_call" 
      + expected - actual

       {
      -  "functionName": [undefined]
      +  "functionName": "collectFiles"
       }
      
      at partialDeepCompare (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/test/testUtil.ts:300:12)
      at assertTelemetry (/codebuild/output/src2630993767/src/github.com/aws/aws-toolkit-vscode/packages/core/src/test/testUtil.ts:351:9)
      at Context.<anonymous> (/codebuild/

Copy link
Contributor

Choose a reason for hiding this comment

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

this file has non-perf tests in it. only the perf tests should be moved

@Hweinstock
Copy link
Contributor Author

For the failing integ tests performance tests, I can raise the thresholds to make them less flaky once I have the filesystem spy. I think the other failing test should be fixed by #5725

@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

Copy link
Contributor

Choose a reason for hiding this comment

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

let's put this in testInteg/amazon/...test.ts. "codewhisperer" is not a name that means anything anymore.

@justinmk3
Copy link
Contributor

WDYT about placing all the performance tests in testInteg/perf/* ? Normally we arrange things to map to the application modules, but it might make sense to group all perf tests in one place because:

  1. in the future, it may eventually be desirable to move them to an entirely different place (e.g. testPerf/ instead of testInteg/perf/)
  2. now and in the future it's likely that we may need to ignore all perf tests, both in CI and locally.
  3. perf tests have a particular "pattern" that they all seem to follow so grouping them may be helpful for others writing new perf tests.

@Hweinstock
Copy link
Contributor Author

Yeah I think that makes total sense. If they are grouped in this way, does it make it easier to ignore them with a pattern or something? It might be nice to do that until I finish the work to count the system calls rather than relying solely on measuring their impact on system statistics implemented.

@justinmk3
Copy link
Contributor

If they are grouped in this way, does it make it easier to ignore them with a pattern or something?

exactly what I'm thinking :)

@Hweinstock
Copy link
Contributor Author

/runIntegrationTests

@justinmk3
Copy link
Contributor

LGTM! Are the failing integ related to this PR? If not let's merge this!

p.s. would you mind adding an item for "Performance tests" here: https://github.com/aws/aws-toolkit-vscode/blob/master/docs/TESTPLAN.md#test-categories

  1) startSecurityScanPerformanceTest
       Should calculate cpu and memory usage for file scans performance tests
         "after all" hook for "Should calculate cpu and memory usage for file scans - test run 10":

      AssertionError [ERR_ASSERTION]: Expected total user CPU usage for Should calculate cpu and memory usage for file scans to be less than 50. Actual user CPU usage was 103.68150661287952
      + expected - actual

      -false
      +true

@Hweinstock
Copy link
Contributor Author

Believe those are just the flakiness of the perf tests. Working on a PR to increase the thresholds and add file system counts to all the tests now to make these less flaky.

Also, will make doc change in follow-up.

@justinmk3 justinmk3 merged commit 1bb57a7 into aws:master Oct 14, 2024
27 of 30 checks passed
@Hweinstock Hweinstock deleted the pTests/moveToInteg branch October 14, 2024 19:19
tverney pushed a commit to tverney/aws-toolkit-vscode that referenced this pull request Oct 21, 2024
## Problem
Performance test involve running the same code 10 times, and often its
demanding code. This should live with integ rather than unit tests to
avoid slow down.

## Solution
- Move
https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/test/amazonqFeatureDev/prepareRepoData.test.ts
into integ folder.
- Move
https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/test/codewhisperer/commands/startSecurityScan.test.ts
into integ folder.
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.

2 participants