Skip to content
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.

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

test(global-writes): add e2e tests COMPASS-8441 #6430

Merged
merged 21 commits into from
Nov 11, 2024

Conversation

mabaasit
Copy link
Contributor

@mabaasit mabaasit commented Nov 1, 2024

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

#
# When setting up for the first time, make sure you:
# - Add payment details to be able to create clusters. You can use stripe test card.
# - Allow network access to the project for your IP address.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hah, just hit me that we can make this step unnecessary also with atlas cli (in this PR)

@@ -46,6 +46,10 @@
#
# (ATLAS_CLOUD_TEST_CLUSTER_NAME="TestCluster" source .evergreen/start-atlas-cloud-cluster.sh \
# && npm run -w compass-e2e-tests test web -- --test-atlas-cloud-sandbox --test-filter="atlas-cloud/**/*")
#
# When setting up for the first time, make sure you:
# - Add payment details to be able to create clusters. You can use stripe test card.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for updating the documentation here! Can you move this to a more logical place where it would fit if you're doing these actions step by step? Probably either as part of creating a new org / project or as a step after it. The intention for the whole instruction above to be about setting up for the first time, so adding it as notes at the bottom of that is a bit weird 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah will do that!


it('should be able to shard an unsharded namespace and also unmanage it', async function () {
// Sharding a collection takes a bit longer
this.timeout(60_000);
Copy link
Collaborator

@gribnoysup gribnoysup Nov 1, 2024

Choose a reason for hiding this comment

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

This is smaller than our default timeout though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤭
will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually bumped it for whole suite

<div className={containerStyles}>
<div
className={containerStyles}
data-testid={`${shardingStatus.toLowerCase()}-status`}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer something like data-status={status} and data-testid=global-writes-container - because status is more of a property of the workspace than an id (for example it could be generic NOT_READY). Alternatively the testid could be a combination of id and status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data-status={status} reads better. i'll change it

@paula-stacho
Copy link
Contributor

By the way, I thought we'd do just one combined path to reduce the amount of setup, but it's not a must.

await browser.clickVisible(Selectors.GlobalWrites.UnmanageNamespaceButton);

// It transitions to the unmanaging state
await waitForGlobalWritesStatus(browser, 'UNSHARDED');
Copy link
Contributor

Choose a reason for hiding this comment

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

note: this will be INCOMPLETE_SHARDING_SETUP after #6399, but it's quite likely that this will be merged first, so I'll update the test afterwards

Copy link
Contributor

Choose a reason for hiding this comment

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

I might even add a test to check that the form is not present at this point, it's important that the key cannot be changed after unmanaging. But again, this hangs on my PR.

@mabaasit
Copy link
Contributor Author

mabaasit commented Nov 1, 2024

By the way, I thought we'd do just one combined path to reduce the amount of setup, but it's not a must.

We are not doing multiple setup here, just running multiple test cases. We can also add test cases for other workflows here if needed.

@gribnoysup gribnoysup force-pushed the COMPASS-8441-global0=-writes-tests branch from 8b3da4a to 94aca71 Compare November 1, 2024 14:29
@gribnoysup gribnoysup force-pushed the COMPASS-8441-global0=-writes-tests branch from 94aca71 to ad2a360 Compare November 1, 2024 14:36
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Neat! Excited to see more e2e tests for these cloud-only features added 🙂

@@ -1400,3 +1400,21 @@ export const ProxyCustomButton =

// Close tab confirmation
export const ConfirmTabCloseModal = '[data-testid="confirm-tab-close"]';

export const GlobalWrites = {
tabStatus: (status: string) => `[data-status="${status.toLowerCase()}"]`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit: I think you might want to follow @paula-stacho advice here and add both data-testid as a special test selector in addition to the data-status, otherwise the selector you're building is a bit too vague about the context of selection (status of what?) which might make it hard to debug

Copy link
Contributor

@paula-stacho paula-stacho left a comment

Choose a reason for hiding this comment

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

Finally got it working, looks good! Thank you! 🎉

@mabaasit mabaasit merged commit 5a3fe53 into main Nov 11, 2024
30 checks passed
@mabaasit mabaasit deleted the COMPASS-8441-global0=-writes-tests branch November 11, 2024 16: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