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.

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

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

Merged
merged 7 commits into from
Oct 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed6acd7
enable test against atlas
syn-zhu Oct 18, 2024
6c17604
use const
syn-zhu Oct 18, 2024
9585f50
update function
syn-zhu Oct 21, 2024
a0b6199
comments
syn-zhu Oct 21, 2024
40b6447
fix if check
syn-zhu Oct 21, 2024
6eb7796
comments
syn-zhu Oct 22, 2024
6b85cd5
comments
syn-zhu Oct 22, 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
59 changes: 50 additions & 9 deletions packages/compass-e2e-tests/helpers/compass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@syn-zhu syn-zhu Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or rather TEST_ATLAS_CLOUD_EXTERNAL_URL_ to match existing ones

export const ATLAS_GROUP_ID = process.env.ATLAS_GROUP_ID;
// multiple connections is now the default
export const TEST_MULTIPLE_CONNECTIONS = true;

Expand Down Expand Up @@ -75,19 +77,22 @@ export const MONGODB_TEST_SERVER_PORT = Number(
process.env.MONGODB_TEST_SERVER_PORT ?? 27091
);

export const DEFAULT_CONNECTION_STRING_1 = `mongodb://127.0.0.1:${MONGODB_TEST_SERVER_PORT}/test`;
export const DEFAULT_CONNECTION_STRING_1 =
process.env.CONNECTION_STRING_1 ||
`mongodb://127.0.0.1:${MONGODB_TEST_SERVER_PORT}/test`;
// NOTE: in browser.setupDefaultConnections() we don't give the first connection an
// explicit name, so it gets a calculated one based off the connection string
export const DEFAULT_CONNECTION_NAME_1 = connectionNameFromString(
DEFAULT_CONNECTION_STRING_1
);
export const DEFAULT_CONNECTION_NAME_1 =
process.env.CONNECTION_NAME_1 ||
connectionNameFromString(DEFAULT_CONNECTION_STRING_1);

// for testing multiple connections
export const DEFAULT_CONNECTION_STRING_2 = `mongodb://127.0.0.1:${
MONGODB_TEST_SERVER_PORT + 1
}/test`;
export const DEFAULT_CONNECTION_STRING_2 =
process.env.CONNECTION_STRING_2 ||
`mongodb://127.0.0.1:${MONGODB_TEST_SERVER_PORT + 1}/test`;
// NOTE: in browser.setupDefaultConnections() the second connection gets given an explicit name
export const DEFAULT_CONNECTION_NAME_2 = 'connection-2';
export const DEFAULT_CONNECTION_NAME_2 =
process.env.CONNECTION_NAME_2 || 'connection-2';

export function updateMongoDBServerInfo() {
try {
Expand Down Expand Up @@ -787,7 +792,43 @@ export async function startBrowser(
...webdriverOptions,
...wdioOptions,
})) as CompassBrowser;
await browser.navigateTo('http://localhost:7777/');

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

const cookiesFile = process.env.COOKIES_FILE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const cookiesFile = process.env.COOKIES_FILE;
const cookiesFile = process.env.TEST_ATLAS_CLOUD_EXTERNAL_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'));
Copy link
Contributor Author

@syn-zhu syn-zhu Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add some type definitions for this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like, for the cookies object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


for (const cookie of cookies) {
if (
cookie.name.includes('mmsa-') ||
cookie.name.includes('mdb-sat') ||
cookie.name.includes('mdb-srt')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

}
}

await browser.navigateTo(
`https://${ATLAS_DOMAIN}/v2/${ATLAS_GROUP_ID}#/explorer`
);
} else {
await browser.navigateTo('http://localhost:7777/');
}

const compass = new Compass(name, browser, {
mode: 'web',
writeCoverage: false,
Expand Down
161 changes: 87 additions & 74 deletions packages/compass-e2e-tests/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
LOG_PATH,
removeUserDataDir,
updateMongoDBServerInfo,
ATLAS_DOMAIN,
} from './helpers/compass';
import ResultLogger from './helpers/result-logger';

Expand Down Expand Up @@ -60,52 +61,54 @@ async function setup() {
const disableStartStop = process.argv.includes('--disable-start-stop');
const shouldTestCompassWeb = process.argv.includes('--test-compass-web');

// When working on the tests it is faster to just keep the server running.
if (!disableStartStop) {
debug('Starting MongoDB server');
crossSpawn.sync('npm', ['run', 'start-servers'], { stdio: 'inherit' });

if (shouldTestCompassWeb) {
debug('Starting Compass Web');
compassWeb = crossSpawn.spawn(
'npm',
['run', '--unsafe-perm', 'start-web'],
{
cwd: path.resolve(__dirname, '..', '..'),
env: {
...process.env,
OPEN_BROWSER: 'false', // tell webpack dev server not to open the default browser
DISABLE_DEVSERVER_OVERLAY: 'true',
APP_ENV: 'webdriverio',
},
if (!ATLAS_DOMAIN) {
// When working on the tests it is faster to just keep the server running.
if (!disableStartStop) {
debug('Starting MongoDB server');
crossSpawn.sync('npm', ['run', 'start-servers'], { stdio: 'inherit' });

if (shouldTestCompassWeb) {
debug('Starting Compass Web');
compassWeb = crossSpawn.spawn(
'npm',
['run', '--unsafe-perm', 'start-web'],
{
cwd: path.resolve(__dirname, '..', '..'),
env: {
...process.env,
OPEN_BROWSER: 'false', // tell webpack dev server not to open the default browser
DISABLE_DEVSERVER_OVERLAY: 'true',
APP_ENV: 'webdriverio',
},
}
);

compassWeb.stdout.pipe(process.stdout);
compassWeb.stderr.pipe(process.stderr);

let serverReady = false;
const start = Date.now();
while (!serverReady) {
if (Date.now() - start >= 120_000) {
throw new Error(
'The compass-web sandbox is still not running after 120000ms'
);
}
try {
const res = await fetch('http://localhost:7777');
serverReady = res.ok;
debug('Web server ready: %s', serverReady);
} catch (err) {
debug('Failed to connect to dev server: %s', (err as any).message);
}
await wait(1000);
}
);

compassWeb.stdout.pipe(process.stdout);
compassWeb.stderr.pipe(process.stderr);

let serverReady = false;
const start = Date.now();
while (!serverReady) {
if (Date.now() - start >= 120_000) {
throw new Error(
'The compass-web sandbox is still not running after 120000ms'
);
}
try {
const res = await fetch('http://localhost:7777');
serverReady = res.ok;
debug('Web server ready: %s', serverReady);
} catch (err) {
debug('Failed to connect to dev server: %s', (err as any).message);
}
await wait(1000);
} else {
debug('Writing electron-versions.json');
crossSpawn.sync('scripts/write-electron-versions.sh', [], {
stdio: 'inherit',
});
}
} else {
debug('Writing electron-versions.json');
crossSpawn.sync('scripts/write-electron-versions.sh', [], {
stdio: 'inherit',
});
}
}

Expand All @@ -118,8 +121,10 @@ async function setup() {

fs.mkdirSync(LOG_PATH, { recursive: true });

debug('Getting mongodb server info');
updateMongoDBServerInfo();
if (!ATLAS_DOMAIN) {
debug('Getting mongodb server info');
updateMongoDBServerInfo();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Various checks in tests rely on this information existing, you can't just skip 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.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

9585f50

}
}

function getResources() {
Expand All @@ -139,34 +144,36 @@ function cleanup() {
const disableStartStop = process.argv.includes('--disable-start-stop');
const shouldTestCompassWeb = process.argv.includes('--test-compass-web');

if (!disableStartStop) {
if (shouldTestCompassWeb) {
debug('Stopping compass-web');
try {
if (compassWeb.pid) {
debug(`killing compass-web [${compassWeb.pid}]`);
kill(compassWeb.pid, 'SIGINT');
} else {
debug('no pid for compass-web');
if (!ATLAS_DOMAIN) {
if (!disableStartStop) {
if (shouldTestCompassWeb) {
debug('Stopping compass-web');
try {
if (compassWeb.pid) {
debug(`killing compass-web [${compassWeb.pid}]`);
kill(compassWeb.pid, 'SIGINT');
} else {
debug('no pid for compass-web');
}
} catch (e) {
debug('Failed to stop compass-web', e);
}
} catch (e) {
debug('Failed to stop compass-web', e);
}
}

debug('Stopping MongoDB server');
try {
crossSpawn.sync('npm', ['run', 'stop-servers'], {
// If it's taking too long we might as well kill the process and move on,
// mongodb-runner is flaky sometimes and in ci `posttest-ci` script will
// take care of additional clean up anyway
timeout: 120_000,
stdio: 'inherit',
});
} catch (e) {
debug('Failed to stop MongoDB Server', e);
debug('Stopping MongoDB server');
try {
crossSpawn.sync('npm', ['run', 'stop-servers'], {
// If it's taking too long we might as well kill the process and move on,
// mongodb-runner is flaky sometimes and in ci `posttest-ci` script will
// take care of additional clean up anyway
timeout: 120_000,
stdio: 'inherit',
});
} catch (e) {
debug('Failed to stop MongoDB Server', e);
}
debug('Done stopping');
}
debug('Done stopping');
}

// Since the webdriverio update something is messing with the terminal's
Expand Down Expand Up @@ -258,9 +265,10 @@ async function main() {

const e2eTestGroupsAmount = parseInt(process.env.E2E_TEST_GROUPS || '1');
const e2eTestGroup = parseInt(process.env.E2E_TEST_GROUP || '1');
const e2eTestFilter = process.env.E2E_TEST_FILTER || '*';

const rawTests = (
await glob('tests/**/*.{test,spec}.ts', {
const tests = (
await glob(`tests/**/${e2eTestFilter}.{test,spec}.ts`, {
cwd: __dirname,
})
).filter((value, index, array) => {
Expand All @@ -271,15 +279,20 @@ async function main() {
return index >= minGroupIndex && index <= maxGroupIndex;
});

console.info('Test files:', rawTests);
console.info('Test files:', tests);

// The only test file that's interested in the first-run experience (at the
// time of writing) is time-to-first-query.ts and that happens to be
// alphabetically right at the end. Which is fine, but the first test to run
// will also get the slow first run experience for no good reason unless it is
// the time-to-first-query.ts test.
// So yeah.. this is a bit of a micro optimisation.
const tests = [FIRST_TEST, ...rawTests.filter((t) => t !== FIRST_TEST)];
for (let i = 0; i < tests.length; ++i) {
if (tests[i] === FIRST_TEST) {
[tests[i], tests[0]] = [tests[0], tests[i]];
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will update

Copy link
Contributor Author

@syn-zhu syn-zhu Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


// Ensure the insert-data mocha hooks are run.
tests.unshift(path.join('helpers', 'insert-data.ts'));
Expand Down
6 changes: 5 additions & 1 deletion packages/compass-e2e-tests/tests/collection-rename.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
screenshotIfFailed,
TEST_COMPASS_WEB,
DEFAULT_CONNECTION_NAME_1,
ATLAS_DOMAIN,
} from '../helpers/compass';
import type { CompassBrowser } from '../helpers/compass-browser';
import { createDummyCollections } from '../helpers/insert-data';
Expand Down Expand Up @@ -84,7 +85,10 @@ describe('Collection Rename Modal', () => {
before(async function () {
compass = await init(this.test?.fullTitle());
browser = compass.browser;
await browser.setupDefaultConnections();
// no need to setup connections if we are running against Atlas
if (!ATLAS_DOMAIN) {
await browser.setupDefaultConnections();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@gribnoysup gribnoysup Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oops, just saw you already suggested that lol

}
});

beforeEach(async function () {
Expand Down
Loading