chore(e2e): enable e2e tests to run directly against Atlas #6381

syn-zhu · 2024-10-18T19:47:59Z

Description

https://jira.mongodb.org/browse/CLOUDP-274595

Enable running compass E2E tests directly against a deployed Atlas environment.

Additionally, enable specifying a test filter as env var to support only running a subset of tests.

Example usage:

export CONNECTION_STRING_1='mongodb+srv://<username>:<password>@connection-1.dkur9.mongodb-dev.net/?retryWrites=true&w=majority&appName=connection-1'
export CONNECTION_NAME_1=connection-1
export CONNECTION_STRING_2='mongodb+srv://<username>:<password>@connection-2.dkur9.mongodb-dev.net/?retryWrites=true&w=majority&appName=connection-2'
export CONNECTION_NAME_2=connection-2
export ATLAS_DOMAIN=cloud-dev.mongodb.com
export ATLAS_GROUP_ID=66ce1716209aca269796919b
export COOKIES_FILE=/Users/siyanzhu/compass/packages/compass-e2e-tests/cloud-dev.mongodb.com_cookies.json
export E2E_TEST_FILTER=collection-rename

npm run test-web

^This would run the collection-rename test against cloud-dev, for group 66ce1716209aca269796919b.

The current implementation assumes that the actual clusters have already been created beforehand. Later on, we will do this step using the MMS e2e utilities.

Currently, the collection-rename test has been tried and is working. Will do each of the remaining tests incrementally, and make any additional changes along the way.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

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)

syn-zhu · 2024-10-18T20:00:25Z

packages/compass-e2e-tests/helpers/compass.ts

+    const cookiesFile = process.env.COOKIES_FILE;
+    if (!cookiesFile) {
+      throw new Error(
+        'ATLAS_DOMAIN is set but COOKIES_FILE is not. Please set COOKIES_FILE to the path of the cookies file.'
+      );
+    }
+    const cookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));


Eventually, the plan is to package npm run test-web as a container image, which can be pulled in MMS e2e task.

MMS already has a bunch of utilities for e2e test setup, in particular:

Creating a new randomly generated test user

logging in as that user

Creating a new group

Creating new clusters in that group

Thus, we don't need to reimplement all of those here. Instead, we can do those setup steps first, and then just save the cookies that are needed for authentication.

Then, when we run the compass e2e test container, we just pass in the cookies, the group, and the names of the created clusters.

syn-zhu · 2024-10-18T20:04:34Z

packages/compass-e2e-tests/helpers/compass.ts

+        cookie.name.includes('mmsa-') ||
+        cookie.name.includes('mdb-sat') ||
+        cookie.name.includes('mdb-srt')


These are the cookies that are relevant for authenticating.

https://github.com/10gen/mms/blob/6d27992a6ab9ab31471c8bcdaa4e347aa39f4013/server/src/features/com/xgen/svc/cukes/helpers/Client.java#L122-L130

Can we add this link as a comment here so that we have the reference and documentation in the code?

gribnoysup · 2024-10-21T13:14:02Z

packages/compass-e2e-tests/helpers/compass.ts

+        'ATLAS_DOMAIN is set but COOKIES_FILE is not. Please set COOKIES_FILE to the path of the cookies file.'
+      );
+    }
+    const cookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));


Can you add some type definitions for this file?

You mean like, for the cookies object?

Yes, something like:

Suggested change

const cookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));

type StoredAtlasCloudCookies = {...some shape}[]

const cookies: StoredAtlasCloudCookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));

We probably don't need any strict validation, but without the type it's unclear what's the expected shape for the content of the COOKIES_FILE file and taking into account that it will be generated in a completely separate repo, it's probably our best way of documenting this

gribnoysup · 2024-10-21T13:16:10Z

packages/compass-e2e-tests/index.ts

-  updateMongoDBServerInfo();
+  if (!ATLAS_DOMAIN) {
+    debug('Getting mongodb server info');
+    updateMongoDBServerInfo();


Various checks in tests rely on this information existing, you can't just skip it

Ah I think I need to update the function to use the variable instead of hardcoded address like it currently does

Yeah, probably can use DEFAULT_CONNECTION_STRING_1 there insteasd of just port, this is aligned more with what's expected

gribnoysup · 2024-10-21T13:16:51Z

packages/compass-e2e-tests/index.ts

+  for (let i = 0; i < tests.length; ++i) {
+    if (tests[i] === FIRST_TEST) {
+      [tests[i], tests[0]] = [tests[0], tests[i]];
+      break;
+    }
+  }


Why is this changed?

Because I added the test filtering feature, and this line previously was always including the FIRST_TEST.

Ah, gotcha, was kinda hard to understand from the code. Maybe better to be a bit more explicit about it?

Suggested change

for (let i = 0; i < tests.length; ++i) {

if (tests[i] === FIRST_TEST) {

[tests[i], tests[0]] = [tests[0], tests[i]];

break;

}

}

const shouldRunAllTests = e2eTestFilter === '*';

const tests = shouldRunAllTests ? <logic to move FIRST_TEST to the front> : <just exactly what glob returned>

Sure, will update

Actually, looking at this again, I think it's more like we want to move time-to-first-query.test.ts to the front only if it is one of the tests we will run (it may still be included even if a filter is used).

So technically, we need to check if it's in the full array, and that point we are already iterating the array, so we probably don't need to disambiguate the two cases here.

Sure, if the intent is to be able to pick up more than one file, this makes sense. Can we rewrite this using a sort then instead of in place swapping so that the code is readable and the intent is clear?

gribnoysup · 2024-10-21T13:22:38Z

packages/compass-e2e-tests/tests/collection-rename.test.ts

-    await browser.setupDefaultConnections();
+    // no need to setup connections if we are running against Atlas
+    if (!ATLAS_DOMAIN) {
+      await browser.setupDefaultConnections();


I guess it's just temporarily only in this file? You'd need to do this in almost every test file and the pattern is to try to keep the test code as similar as possible (not to mention that changing this logic if we start adding things like that everywhere would be very hard)

Well, in Atlas when you create new clusters, then the connections start showing up automatically in Data Explore. So there is no extra step needed to "setup" the connections via the connection creation modal.

So, I actually do think we would skip this step for every test if we're running against Atlas. Are there any concerns about this?

If switch statements are generic like that and would apply to every test case, we add them in the helper methods themselves, not in the test case, so again, if this is just temporary because that's the only test case you are adding, that's fine, but for follow-up PRs we'd need to move this logic out of test cases

Maybe I can add this check into the function itself? Would that be better?

Oh oops, just saw you already suggested that lol

gribnoysup · 2024-10-21T13:23:45Z

packages/compass-e2e-tests/helpers/compass.ts

@@ -46,6 +46,8 @@ let MONGODB_USE_ENTERPRISE =

 // should we test compass-web (true) or compass electron (false)?
 export const TEST_COMPASS_WEB = process.argv.includes('--test-compass-web');
+export const ATLAS_DOMAIN = process.env.ATLAS_DOMAIN;


For all these new variables you're adding, can we make the names more distinct to indicate the use-case better? Especially the CONNECTION_ ones, a lot of things rely on these, so just allowing arbitrary overrides without context is not great

Sure, do you have some concrete suggestions?

For the connection_ ones in particular, do you just mean adding ATLAS_ to the name or something?

And for this one as well, let me know how I can make it more specific.

Yeah, a common prefix we can stick on all of them is probably the way to go, something like ATLAS_CLOUD_EXTERNAL_URL_TESTING_, just so it's clear it's specifically for this "testing a deployed atlas cloud" scenario, not a generic way to control these variables

Or rather TEST_ATLAS_CLOUD_EXTERNAL_URL_ to match existing ones

gribnoysup · 2024-10-22T08:03:29Z

packages/compass-e2e-tests/helpers/compass.ts

+        process.env.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_STRING_1 ||
+          `mongodb://127.0.0.1:${String(MONGODB_TEST_SERVER_PORT)}`,


Suggested change

process.env.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_STRING_1 ||

`mongodb://127.0.0.1:${String(MONGODB_TEST_SERVER_PORT)}`,

DEFAULT_CONNECTION_STRING_1

The /test suffix doesn't make a difference here?

DEFAULT_CONNECTION_STRING_1 has the /test suffix in its definition, but the one here doesn't.

I think that's supposed to refer to the auth DB that is used? if you think it won't make a difference then I can change it to what you suggested.

Yep, doesn't make a difference, it's a valid connection string

gribnoysup · 2024-10-22T08:03:54Z

packages/compass-e2e-tests/helpers/compass.ts

+    // Navigate to a 404 page to set cookies
+    await browser.navigateTo(`https://${TEST_ATLAS_CLOUD_EXTERNAL_URL}/404`);


Actually, not sure I understand, why are we doing this if we're setting cookies from a file below? Can we just use all the cookies from a file?

The API only allows you to set cookies for the domain you're actually on haha

idk why, but it's in the documentation here: https://webdriver.io/docs/api/browser/setCookies/

Ah, okay, I thought we're going on this page so that the set-cookie header from the backend can set some cookies, not so that the browser.setCookies call can work, got it!

gribnoysup · 2024-10-22T08:04:12Z

packages/compass-e2e-tests/helpers/compass.ts

+    // Navigate to a 404 page to set cookies
+    await browser.navigateTo(`https://${TEST_ATLAS_CLOUD_EXTERNAL_URL}/404`);
+
+    const cookiesFile = process.env.COOKIES_FILE;


Suggested change

const cookiesFile = process.env.COOKIES_FILE;

const cookiesFile = process.env.TEST_ATLAS_CLOUD_EXTERNAL_COOKIES_FILE;

gribnoysup · 2024-10-22T10:34:11Z

packages/compass-e2e-tests/helpers/compass.ts

+        await browser.setCookies({
+          name: cookie.name,
+          value: cookie.value,
+          domain: cookie.domain,
+          path: cookie.path,
+          secure: cookie.secure,
+          httpOnly: cookie.httpOnly,
+        });


setCookies takes an array, you don't need to do this in sequence

gribnoysup

We should rename the rest of some specific Atlas Cloud test related variables (cookie file is the only one left I think, at least the one I noticed on re-review), otherwise a few nits, but looks good to merge

…runner This also includes re-applying changes from #6381 from scratch

enable test against atlas

ed6acd7

syn-zhu requested review from lerouxb, gribnoysup and Anemy October 18, 2024 19:51

syn-zhu commented Oct 18, 2024

View reviewed changes

use const

6c17604

gribnoysup reviewed Oct 21, 2024

View reviewed changes

syn-zhu requested a review from gribnoysup October 21, 2024 14:08

syn-zhu added 2 commits October 21, 2024 10:15

update function

9585f50

comments

a0b6199

syn-zhu requested review from gribnoysup and removed request for gribnoysup October 21, 2024 20:07

fix if check

40b6447

gribnoysup reviewed Oct 22, 2024

View reviewed changes

gribnoysup approved these changes Oct 22, 2024

View reviewed changes

syn-zhu added 2 commits October 22, 2024 09:33

comments

6eb7796

comments

6b85cd5

syn-zhu merged commit add5de6 into main Oct 22, 2024
22 of 25 checks passed

syn-zhu deleted the run-e2e-against-atlas branch October 22, 2024 16:04

gribnoysup added a commit that referenced this pull request Oct 23, 2024

Merge remote-tracking branch 'origin/main' into restructure-e2e-test-…

c921216

…runner This also includes re-applying changes from #6381 from scratch

	const cookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));
	type StoredAtlasCloudCookies = {...some shape}[]
	const cookies: StoredAtlasCloudCookies = JSON.parse(await fs.readFile(cookiesFile, 'utf8'));

		process.env.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_STRING_1 \|\|
		`mongodb://127.0.0.1:${String(MONGODB_TEST_SERVER_PORT)}`,

	process.env.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_STRING_1 \|\|
	`mongodb://127.0.0.1:${String(MONGODB_TEST_SERVER_PORT)}`,
	DEFAULT_CONNECTION_STRING_1

		// Navigate to a 404 page to set cookies
		await browser.navigateTo(`https://${TEST_ATLAS_CLOUD_EXTERNAL_URL}/404`);

	const cookiesFile = process.env.COOKIES_FILE;
	const cookiesFile = process.env.TEST_ATLAS_CLOUD_EXTERNAL_COOKIES_FILE;

chore(e2e): enable e2e tests to run directly against Atlas #6381

chore(e2e): enable e2e tests to run directly against Atlas #6381

Conversation

syn-zhu commented Oct 18, 2024 • edited Loading

Description

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

syn-zhu Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syn-zhu Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syn-zhu Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup left a comment

Choose a reason for hiding this comment

syn-zhu commented Oct 18, 2024 •

edited

Loading

syn-zhu Oct 18, 2024 •

edited

Loading

syn-zhu Oct 21, 2024 •

edited

Loading

gribnoysup Oct 21, 2024 •

edited

Loading

syn-zhu Oct 21, 2024 •

edited

Loading

gribnoysup Oct 22, 2024 •

edited

Loading