-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add Sharding Support to PSMDB Demand Backup Test #918
base: main
Are you sure you want to change the base?
Add Sharding Support to PSMDB Demand Backup Test #918
Conversation
ui/apps/everest/.e2e/release/demand-backup-psmdb-sharding.e2e.ts
Outdated
Show resolved
Hide resolved
ui/apps/everest/.e2e/release/demand-backup-psmdb-sharding.e2e.ts
Outdated
Show resolved
Hide resolved
ui/apps/everest/.e2e/release/demand-backup-psmdb-sharding.e2e.ts
Outdated
Show resolved
Hide resolved
); | ||
}; | ||
|
||
export const validateMongoDBSharding = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add parameter to this function for collection so you can just call it twice in the test with different collection parameter so that you don't duplicate the code for t1 and t2.
); | ||
|
||
// Shard t1 | ||
await queryPSMDB(cluster, namespace, 'test', 'db.t1.createIndex({ a: 1 });'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move createIndex
to prepareMongoDBTestDB
function.
); | ||
|
||
// Shard t2 | ||
await queryPSMDB(cluster, namespace, 'test', 'db.t2.createIndex({ b: 1 });'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move createIndex
to prepareMongoDBTestDB
function.
cluster, | ||
namespace, | ||
'test', | ||
'db.t2.insertMany([{ b: 1 }, { b: 2 }, { b: 3 }]);' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please call the keys the same as in t1, so { a: 1 }, { a: 2 }, { a: 3 }
. It will simplify things because we will have 1 function for configureMongoSharding which will just take parameter for collection.
); | ||
}; | ||
|
||
export const configureMongoDBSharding = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add parameter for collection name to this function and call it twice so that there is no code duplication for t1 and t2.
enableSharding
can be called in the test directly or it can be left here because I don't think it will have any effect if the sharding is already enabled on the collection.
queryTestDB, | ||
} from '@e2e/utils/db-cmd-line'; | ||
|
||
export let isShardingEnabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems strange exporting this from the test and importing it into helper functions. Let's create a function psmdbShardingEnabled
in db-cmd-line.ts and then use it inside the same file.
const isChecked = await shardingCheckbox?.isChecked(); | ||
if (!isChecked) { | ||
if (shardingCheckbox) { | ||
isShardingEnabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we create a new function in db-cmd-line.ts we don't need this here. Also we want to check if sharding is actually enabled in the backend (api call) and not did we just click on something in UI.
// await moveForward(page); | ||
}); | ||
|
||
// Step to set number of shards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment since test description below has the same.
//if (db != 'psmdb') { | ||
// expect(addedCluster?.spec.proxy.replicas).toBe(size); | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
); | ||
expect(addedCluster?.spec.engine.storage.size.toString()).toBe('1Gi'); | ||
expect(addedCluster?.spec.proxy.expose.type).toBe('internal'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add something like:
expect(addedCluster?.spec.sharding.enabled).toBe(true)
expect(addedCluster?.spec.sharding.shards).toBe(2)
expect(addedCluster?.spec.sharding.configServer.replicas).toBe(3)
expect(addedCluster?.spec.proxy.replicas).toBe(3)
expect(storageClasses.length).toBeGreaterThan(0); | ||
|
||
await page.goto('/databases'); | ||
await page.getByTestId('add-db-cluster-button').waitFor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this part should be moved to utils to avoid duplication 🤔 we call the cluster creation button in a lot of places
Demand backup test for PSMDB to include sharding:
The reference script for this implementation was: demand-backup.e2e.ts