Move end-to-end-tests mocha config to a more conventional location, and include benchmarks in correctness mode#26737
Conversation
…nd include benchmarks in correctness more
There was a problem hiding this comment.
Pull request overview
Updates E2E/benchmark test configuration so benchmark scenarios can also run in correctness mode, and adjusts Mocha configuration placement/usage.
Changes:
- Added performance-mode gating to choose different default E2E document sizes for performance vs correctness runs.
- Adjusted benchmark specs to avoid artificial delays outside performance-mode runs.
- Updated Mocha config/script wiring and introduced a new benchmark dependency.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test/test-version-utils/src/describeE2eDocs.ts | Selects different default E2E doc sets depending on performance-testing mode. |
| packages/test/test-version-utils/package.json | Adds dependency used to detect performance-testing mode. |
| packages/test/test-end-to-end-tests/src/test/benchmark/summarizationDocument.all.spec.ts | Skips a pre-test delay unless running in performance-testing mode. |
| packages/test/test-end-to-end-tests/src/test/benchmark/LoadDocument.all.spec.ts | Skips a pre-test delay unless running in performance-testing mode. |
| packages/test/test-end-to-end-tests/src/test/.mocharc.cjs | Updates how packageDir is computed for shared Mocha config. |
| packages/test/test-end-to-end-tests/package.json | Simplifies test script (now relies on default Mocha config discovery). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
| this.summarizerClient = undefined; | ||
| await delay(2000); | ||
| if (isInPerformanceTestingMode) { | ||
| // TODO: this should be removed, or document why it exists (probably a workaround for memory measurement issues in current version of benchmark). |
There was a problem hiding this comment.
I am not confident in that guess at the motivation, hence the TODO.
#26638 likely fixes the need for this, so this likely won't linger forever
There was a problem hiding this comment.
Before merging we should run the build-client + e2e-tests pipelines on the internal project for this change, I'm wondering if moving this file will cause the execution of e2e tests in the pipeline (where the package is installed from tarball, not run from the repo) to fail because the mocha config didn't get packed.
There was a problem hiding this comment.
Looking at the .npmignore it seems like it should be fine to pull the file to the root of the package, but a sanity check would be nice.
Description
Move end-to-end-tests mocha config to a more conventional location, and include benchmarks in correctness mode.
Reviewer Guidance
The review process is outlined on this wiki page.