Skip to content

chore(e2e-tests): update server aggregation error warning for latest alpha COMPASS-10839 - #8239

Merged
Anemy merged 8 commits into
mainfrom
COMPASS-10839-latest-alpha-sleep-error
Jul 24, 2026
Merged

chore(e2e-tests): update server aggregation error warning for latest alpha COMPASS-10839#8239
Anemy merged 8 commits into
mainfrom
COMPASS-10839-latest-alpha-sleep-error

Conversation

@Anemy

@Anemy Anemy commented Jul 15, 2026

Copy link
Copy Markdown
Member

COMPASS-10839

Updated the long running aggregation to use a CPU bound long running query.
Added comments to the aggregation runs so we can differentiate the preview from a run for the test.

There was a bug in alpha for a bit where things wouldn't cancel when disconnecting. That thread with Neal is also where this long running query approach is mentioned: https://mongodb.slack.com/archives/CKABWR2CT/p1783457292659529

Evergreen run with newest alpha: https://spruce.corp.mongodb.com/version/6a639cd3ce47b000076ee381/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC

Copilot AI review requested due to automatic review settings July 15, 2026 15:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates an e2e server-warning allowlist to keep aggregation-cancellation tests stable against newer server alpha behavior, while preserving signal from unexpected server warnings.

Changes:

  • Allows ClientDisconnect in addition to Interrupted for the relevant aggregate command executor warning (id === 23799) during cancellation.
  • Adjusts the warning-filter predicate formatting in the affected test.

Comment thread packages/compass-e2e-tests/tests/collection-aggregations-tab.test.ts Outdated

// Used in the `comment` on the aggregate command to help identify
// the operation in server logs and currentOp.
export const PREVIEW_AGGREGATION_COMMENT = 'Compass: Aggregation preview';

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm thinking we should use appName instead of a hardcoded Compass here. It will take more wiring around of the appName though as it would be a service and isn't available in these plugins yet. We can also clean up the hardcoded appName passed to CompassAssistantDrawerWithConnections then as well. I'm planning to do it as a follow up.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

all commands can be traced to the originating client and its appName so while it may not be printed in the same logline as the command the information exists and I think that's good enough.

I may go as far to say we shouldn't repeat the appname just for clutter reasons, its two long uuids usually

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Chatted on slack a touch. Sounds good, I think we could even drop the Compass: part. I'll update.
FWIW the appName only gets the two uuids when we pass it to the driver connection options. In the app it's MongoDB Compass or Data Explorer (also MongoDB Compass ReadOnly etc) No follow up needed.

@Anemy
Anemy requested a review from Copilot July 24, 2026 17:36
@Anemy
Anemy marked this pull request as ready for review July 24, 2026 17:37
@Anemy
Anemy requested a review from a team as a code owner July 24, 2026 17:37
@Anemy
Anemy requested a review from paula-stacho July 24, 2026 17:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

packages/compass-e2e-tests/tests/collection-aggregations-tab.test.ts:1060

  • The long-running aggregation builds a CPU-bound expression but runs it against the default numbers collection (1000 docs). Without a $limit, the server may have to begin evaluating this expensive expression across many documents before the cancellation takes effect, which can make the test unnecessarily slow/flaky and can monopolize CPU on the test server.

Consider adding an explicit $limit: 1 stage before the slow $addFields so only a single document is involved while still exercising cancellation.

      // Set first stage to a very slow $addFields
      await browser.selectStageOperator(0, '$addFields');
      await browser.setCodemirrorEditorValue(
        Selectors.stageEditor(0),
        slowQuery
      );

Comment thread packages/compass-aggregations/src/modules/aggregation.spec.ts
@codeowners-service-app

Copy link
Copy Markdown

Assigned ivandevp for team compass-developers because paula-stacho is out of office.


// Used in the `comment` on the aggregate command to help identify
// the operation in server logs and currentOp.
export const PREVIEW_AGGREGATION_COMMENT = 'Compass: Aggregation preview';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

all commands can be traced to the originating client and its appName so while it may not be printed in the same logline as the command the information exists and I think that's good enough.

I may go as far to say we shouldn't repeat the appname just for clutter reasons, its two long uuids usually

@Anemy
Anemy merged commit 24953f0 into main Jul 24, 2026
79 of 80 checks passed
@Anemy
Anemy deleted the COMPASS-10839-latest-alpha-sleep-error branch July 24, 2026 22:51
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