Skip to content

Conversation

@lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Nov 12, 2025

I filed one follow-up ticket for something I couldn't quickly fix.

Copilot AI review requested due to automatic review settings November 12, 2025 10:04
@lerouxb lerouxb requested a review from a team as a code owner November 12, 2025 10:04
@lerouxb lerouxb requested a review from johnjackweir November 12, 2025 10:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates test assertions across the codebase to call .to.throw() instead of using .to.throw as a property accessor. This change fixes incorrect test syntax where expect statements were checking if .throw exists as a property rather than actually asserting that functions throw errors.

Key Changes:

  • Fixed 100+ instances of .to.throw to properly call .to.throw()
  • One bug fix in item-actions.ts correcting logic from && to ||

Reviewed Changes

Copilot reviewed 46 out of 46 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/databases-collections/src/stores/create-namespace.spec.tsx Fixed 4 throw assertions
packages/connection-form/src/components/connection-form.spec.tsx Fixed 7 throw assertions
packages/connection-form/src/components/advanced-options-tabs/csfle-tab/csfle-tab.spec.tsx Fixed 2 throw assertions
packages/connection-form/src/components/advanced-options-tabs/advanced-tab/url-options-list-editor.spec.tsx Fixed 1 throw assertion
packages/compass-sidebar/src/components/multiple-connections/sidebar.spec.tsx Fixed 1 throw assertion
packages/compass-shell/src/modules/history-storage.spec.ts Fixed 1 throw assertion
packages/compass-settings/src/stores/preferences-sandbox.spec.ts Fixed 1 throw assertion
packages/compass-saved-aggregations-queries/src/index.spec.tsx Fixed 3 throw assertions
packages/compass-preferences-model/src/preferences.spec.ts Fixed 1 throw assertion
packages/compass-preferences-model/src/preferences-persistent-storage.spec.ts Fixed 2 throw assertions
packages/compass-indexes/src/components/search-indexes-table/search-indexes-table.spec.tsx Fixed 3 throw assertions
packages/compass-indexes/src/components/regular-indexes-table/type-field.spec.tsx Fixed 1 throw assertion
packages/compass-indexes/src/components/regular-indexes-table/regular-indexes-table.spec.tsx Fixed 10 throw assertions
packages/compass-indexes/src/components/regular-indexes-table/property-field.spec.tsx Fixed 1 throw assertion
packages/compass-indexes/src/components/regular-indexes-table/index-actions.spec.tsx Fixed 1 throw assertion
packages/compass-indexes/src/components/indexes/indexes.spec.tsx Fixed 1 throw assertion
packages/compass-indexes/src/components/indexes-toolbar/indexes-toolbar.spec.tsx Fixed 1 throw assertion
packages/compass-crud/src/components/virtualized-document-list-view.spec.tsx Fixed 2 throw assertions
packages/compass-crud/src/components/virtualized-document-json-view.spec.tsx Fixed 4 throw assertions
packages/compass-context-menu/src/use-context-menu.spec.tsx Fixed 4 throw assertions
packages/compass-connections/src/stores/connections-store-redux.spec.tsx Fixed 1 throw assertion
packages/compass-connections-navigation/src/item-actions.ts Fixed logic bug: changed && to ||
packages/compass-connections-navigation/src/connections-navigation-tree.spec.tsx Fixed 13 throw assertions
packages/compass-components/src/components/virtual-list.spec.tsx Fixed 2 throw assertions
packages/compass-components/src/components/guide-cue/guide-cue.spec.tsx Fixed 4 throw assertions
packages/compass-components/src/components/document-list/document.spec.tsx Fixed 6 throw assertions
packages/compass-app-stores/src/instances-manager.spec.ts Fixed 2 throw assertions
packages/compass-aggregations/src/stores/create-view.spec.ts Fixed 1 throw assertion
packages/compass-aggregations/src/modules/pipeline-builder/pipeline-preview-manager.spec.ts Fixed 1 throw assertion
packages/compass-aggregations/src/modules/pipeline-builder/pipeline-parser/pipeline-parser.spec.ts Fixed 1 throw assertion
packages/compass-aggregations/src/modules/pipeline-builder/pipeline-builder.spec.ts Fixed 1 throw assertion
packages/compass-aggregations/src/components/saved-pipelines/saved-pipelines.spec.tsx Fixed 2 throw assertions
packages/compass-aggregations/src/components/pipeline-toolbar/pipeline-settings/pipeline-menus.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-toolbar/pipeline-header/pipeline-stages.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-toolbar/index.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-results-workspace/pipeline-results-list.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-results-workspace/pipeline-pagination.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-builder-workspace/pipeline-as-text-workspace/pipeline-stages-preview.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-builder-workspace/pipeline-as-text-workspace/pipeline-preview.spec.tsx Fixed 10 throw assertions
packages/compass-aggregations/src/components/pipeline-builder-workspace/pipeline-as-text-workspace/index.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/pipeline-builder-workspace/index.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/focus-mode/focus-mode.spec.tsx Fixed 2 throw assertions
packages/compass-aggregations/src/components/focus-mode/focus-mode-stage-editor.spec.tsx Fixed 3 throw assertions
packages/compass-aggregations/src/components/aggregation-side-panel/stage-wizard-use-cases/match/match-group-form.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/aggregation-side-panel/stage-wizard-use-cases/group/group-with-subset.spec.tsx Fixed 1 throw assertion
packages/compass-aggregations/src/components/add-stage/add-stage.spec.tsx Fixed 1 throw assertion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

ty for tackling this, we ran into the same problem on node ages ago. We modified the chai throw assertion to require an argument. (which doesn't help if no one calls it, that we didn't solve for, just had a PR like this where we searched and fixed)

@lerouxb
Copy link
Contributor Author

lerouxb commented Nov 12, 2025

ty for tackling this, we ran into the same problem on node ages ago. We modified the chai throw assertion to require an argument. (which doesn't help if no one calls it, that we didn't solve for, just had a PR like this where we searched and fixed)

@nbbeeken I also filed https://jira.mongodb.org/browse/COMPASS-10060 to try and fix this in an eslint rule. Maybe we should list that as an alternative solution there?

@nbbeeken
Copy link
Collaborator

Maybe we should list that as an alternative solution there?

Updated the ticket! It would be an additional requirement, it isn't going to help when the assertion isn't invoked but when it is, if you don't pick a specific error to expect then you can start throwing for a different reason (like ReferenceError) and your test still passes.

// With signal part of streams pipeline the file is created and if
// the signal is aborted the stream is destroyed and file is not
// writable anymore and as a result its not able to write trailing ] to the file.
expect(err.message).to.not.equal('Expected file to not be valid JSON');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by, but the expect.fail would have just been caught and ignored here.

} catch {
// noop
} catch (err: any) {
expect(err.message).to.not.equal(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by

} catch {
// noop
} catch (err: any) {
expect(err.message).to.not.equal(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by

@lerouxb lerouxb changed the title chore: call all to throw COMPASS-10059 chore: call all to.throw; and to.not.throw; COMPASS-10059 Nov 13, 2025
expect(childOnAction).to.have.been.calledOnceWithExactly(1);
expect(parentOnAction).to.not.have.been.called;
expect(() => screen.getByTestId('test-menu')).to.throw;
// TODO(COMPASS-10075)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end I only filed this one follow-up. Trying to fix this was taking lots of time.

@lerouxb lerouxb requested a review from nbbeeken November 14, 2025 18:53
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.

4 participants