Skip to content

fix(compass-global-writes): handle loading error COMPASS-8446 #6451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 11, 2024
Merged

fix(compass-global-writes): handle loading error COMPASS-8446 #6451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12ca3be
fix(compass-global-writes): handle loading error COMPASS-8446
paula-stacho Nov 1, 2024
b2a6afa
add tests
paula-stacho Nov 6, 2024
a426adf
fix polling
paula-stacho Nov 7, 2024
deab820
add test
paula-stacho Nov 7, 2024
9ef0db6
adjustments
paula-stacho Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/compass-global-writes/src/components/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import ShardKeyInvalid from './states/shard-key-invalid';
import ShardKeyMismatch from './states/shard-key-mismatch';
import ShardingError from './states/sharding-error';
import IncompleteShardingSetup from './states/incomplete-sharding-setup';
import LoadingError from './states/loading-error';

const containerStyles = css({
display: 'flex',
Expand Down Expand Up @@ -90,6 +91,10 @@ function ShardingStateView({
return <IncompleteShardingSetup />;
}

if (shardingStatus === ShardingStatuses.LOADING_ERROR) {
return <LoadingError />;
}

return null;
}

Expand Down
20 changes: 20 additions & 0 deletions packages/compass-global-writes/src/components/states/loading-error.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react';
import { expect } from 'chai';
import { screen } from '@mongodb-js/testing-library-compass';
import { LoadingError } from './loading-error';
import { renderWithStore } from '../../../tests/create-store';

const error = 'Test failure';

function renderWithProps(
props?: Partial<React.ComponentProps<typeof LoadingError>>
) {
return renderWithStore(<LoadingError error={error} {...props} />);
}

describe('LoadingError', function () {
it('renders the error', async function () {
await renderWithProps();
expect(screen.getByText(error)).to.exist;
});
});
26 changes: 26 additions & 0 deletions packages/compass-global-writes/src/components/states/loading-error.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import React from 'react';
import { ErrorSummary } from '@mongodb-js/compass-components';
import { connect } from 'react-redux';
import { type RootState, ShardingStatuses } from '../../store/reducer';
import { containerStyles } from '../common-styles';

interface LoadingErrorProps {
error: string;
}

export function LoadingError({ error }: LoadingErrorProps) {
return (
<div className={containerStyles}>
<ErrorSummary errors={error} />
</div>
);
}

export default connect((state: RootState) => {
if (state.status !== ShardingStatuses.LOADING_ERROR) {
throw new Error('Error not found in LoadingError');
}
return {
error: state.loadingError,
};
})(LoadingError);
94 changes: 94 additions & 0 deletions packages/compass-global-writes/src/store/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ function createStore({
hasShardKey = () => false,
failsOnShardingRequest = () => false,
failsOnShardZoneRequest = () => false,
failsToFetchClusterDetails = () => false,
failsToFetchDeploymentStatus = () => false,
failsToFetchShardKey = () => false,
authenticatedFetchStub,
}:
| {
Expand All @@ -81,6 +84,9 @@ function createStore({
hasShardKey?: () => boolean | AtlasShardKey;
failsOnShardingRequest?: () => boolean;
failsOnShardZoneRequest?: () => boolean;
failsToFetchClusterDetails?: () => boolean;
failsToFetchDeploymentStatus?: () => boolean;
failsToFetchShardKey?: () => boolean;
authenticatedFetchStub?: never;
}
| {
Expand All @@ -89,6 +95,9 @@ function createStore({
hasShardKey?: () => boolean | ShardKey;
failsOnShardingRequest?: never;
failsOnShardZoneRequest?: () => never;
failsToFetchClusterDetails?: never;
failsToFetchDeploymentStatus?: never;
failsToFetchShardKey?: () => boolean;
Comment on lines 96 to +100
Copy link
Collaborator

@gribnoysup gribnoysup Nov 8, 2024

Choose a reason for hiding this comment

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

This special test api being a catch-all for all the tests in this file is getting a bit hard to read from my perspective, especially because it seems like most of these methods are added for just one test case. If you need a one-off mocking / stubbing for a special test case like that I would really recommend doing it right in your test:

it('fails when ...', function() {
  sinon.stub(atlasService, 'authenticatedFetch').rejects(...)
  // ... more of the test here
})

That way you're not binding your test to a shared mock implementation which usually makes tests easier to maintain in the long run and easier to understand what's being mocked when reading through the test case

Copy link
Collaborator Author

@paula-stacho paula-stacho Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I get your point, but in this case the tests were getting very verbose and difficult to read because 'authenticatedFetch' handles 4 different things. So often the tests were repeating the whole auth fetch mock logic section only to change 1 thing. The only one that would be easy to mock and only used once would be failsToFetchClusterDetails - because that is the very first request that happens in the flow, and it's only on init. That one I did just for consistency.

authenticatedFetchStub?: () => void;
} = {}): GlobalWritesStore {
const atlasService = {
Expand All @@ -98,6 +107,9 @@ function createStore({
}

if (uri.includes('/clusters/')) {
if (failsToFetchClusterDetails()) {
return Promise.reject(new Error('Failed to fetch cluster details'));
}
return createAuthFetchResponse({
...clusterDetails,
geoSharding: {
Expand All @@ -108,6 +120,9 @@ function createStore({
}

if (uri.includes('/deploymentStatus/')) {
if (failsToFetchDeploymentStatus()) {
return Promise.reject(new Error('Failed to fetch deployment status'));
}
return createAuthFetchResponse({
automationStatus: {
processes: hasShardingError() ? [failedShardingProcess] : [],
Expand All @@ -130,6 +145,10 @@ function createStore({
}),
automationAgentAwait: (_meta: unknown, type: string) => {
if (type === 'getShardKey') {
if (failsToFetchShardKey()) {
return Promise.reject(new Error('Failed to fetch shardKey'));
}

const shardKey = hasShardKey();
return {
response:
Expand Down Expand Up @@ -188,6 +207,35 @@ describe('GlobalWritesStore Store', function () {
});

context('scenarios', function () {
context('initial load fail', function () {
it('fails to fetch cluster details', async function () {
const store = createStore({
failsToFetchClusterDetails: () => true,
});
await waitFor(() => {
expect(store.getState().status).to.equal('LOADING_ERROR');
});
});

it('fails to fetch shard key', async function () {
const store = createStore({
failsToFetchShardKey: () => true,
});
await waitFor(() => {
expect(store.getState().status).to.equal('LOADING_ERROR');
});
});

it('fails to fetch deployment status', async function () {
const store = createStore({
failsToFetchDeploymentStatus: () => true,
});
await waitFor(() => {
expect(store.getState().status).to.equal('LOADING_ERROR');
});
});
});

it('not managed -> sharding -> valid shard key', async function () {
let mockShardKey = false;
let mockManagedNamespace = false;
Expand Down Expand Up @@ -291,6 +339,52 @@ describe('GlobalWritesStore Store', function () {
});
});

context('pulling fail', function () {
it('sharding -> error (failed to fetch shard key)', async function () {
let mockFailure = false;
// initial state === sharding
clock = sinon.useFakeTimers({
shouldAdvanceTime: true,
});
const store = createStore({
isNamespaceManaged: () => true,
failsToFetchShardKey: Sinon.fake(() => mockFailure),
});
await waitFor(() => {
expect(store.getState().status).to.equal('SHARDING');
});

// sharding ends with a request failure
mockFailure = true;
clock.tick(POLLING_INTERVAL);
await waitFor(() => {
expect(store.getState().status).to.equal('LOADING_ERROR');
});
});

it('sharding -> error (failed to fetch deployment status)', async function () {
let mockFailure = false;
// initial state === sharding
clock = sinon.useFakeTimers({
shouldAdvanceTime: true,
});
const store = createStore({
isNamespaceManaged: () => true,
failsToFetchDeploymentStatus: Sinon.fake(() => mockFailure),
});
await waitFor(() => {
expect(store.getState().status).to.equal('SHARDING');
});

// sharding ends with a request failure
mockFailure = true;
clock.tick(POLLING_INTERVAL);
await waitFor(() => {
expect(store.getState().status).to.equal('LOADING_ERROR');
});
});
});

it('sharding -> cancelling request -> not managed', async function () {
let mockManagedNamespace = true;
confirmationStub.resolves(true);
Expand Down
Loading
Loading