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

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

Merged
merged 7 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
7 changes: 7 additions & 0 deletions packages/compass-e2e-tests/helpers/commands/connect-form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
DEFAULT_CONNECTION_NAME_2,
DEFAULT_CONNECTION_STRING_1,
DEFAULT_CONNECTION_STRING_2,
TEST_ATLAS_CLOUD_EXTERNAL_URL,
TEST_MULTIPLE_CONNECTIONS,
} from '../compass';
import Debug from 'debug';
Expand Down Expand Up @@ -986,6 +987,12 @@ export async function setupDefaultConnections(browser: CompassBrowser) {
whereas we do have some tests that try and use those. We can easily change
this in future if needed, though.
*/

// no need to setup connections if we are running against Atlas
if (TEST_ATLAS_CLOUD_EXTERNAL_URL) {
return;
}

for (const connectionName of [
DEFAULT_CONNECTION_NAME_1,
DEFAULT_CONNECTION_NAME_2,
Expand Down
76 changes: 66 additions & 10 deletions packages/compass-e2e-tests/helpers/compass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ 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 TEST_ATLAS_CLOUD_EXTERNAL_URL =
process.env.TEST_ATLAS_CLOUD_EXTERNAL_URL;
export const TEST_ATLAS_CLOUD_EXTERNAL_GROUP_ID =
process.env.TEST_ATLAS_CLOUD_EXTERNAL_GROUP_ID;
// multiple connections is now the default
export const TEST_MULTIPLE_CONNECTIONS = true;

Expand Down Expand Up @@ -75,19 +79,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.TEST_ATLAS_CLOUD_EXTERNAL_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.TEST_ATLAS_CLOUD_EXTERNAL_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.TEST_ATLAS_CLOUD_EXTERNAL_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.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_NAME_2 || 'connection-2';

export function updateMongoDBServerInfo() {
try {
Expand All @@ -106,7 +113,8 @@ export function updateMongoDBServerInfo() {
'server-info',
'--',
'--connectionString',
`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)}`,
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
process.env.TEST_ATLAS_CLOUD_EXTERNAL_CONNECTION_STRING_1 ||
`mongodb://127.0.0.1:${String(MONGODB_TEST_SERVER_PORT)}`,
DEFAULT_CONNECTION_STRING_1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

],
{ encoding: 'utf-8' }
);
Expand Down Expand Up @@ -761,6 +769,16 @@ async function startCompassElectron(
return compass;
}

export type StoredAtlasCloudCookies = {
name: string;
value: string;
domain: string;
path: string;
secure: boolean;
httpOnly: boolean;
expirationDate: number;
}[];

export async function startBrowser(
name: string,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
Expand All @@ -787,7 +805,45 @@ export async function startBrowser(
...webdriverOptions,
...wdioOptions,
})) as CompassBrowser;
await browser.navigateTo('http://localhost:7777/');

if (TEST_ATLAS_CLOUD_EXTERNAL_URL) {
// Navigate to a 404 page to set cookies
await browser.navigateTo(`https://${TEST_ATLAS_CLOUD_EXTERNAL_URL}/404`);
Comment on lines +809 to +810
Copy link
Collaborator

@gribnoysup gribnoysup Oct 22, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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/

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!


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: StoredAtlasCloudCookies = JSON.parse(
await fs.readFile(cookiesFile, 'utf8')
);

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.

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://${TEST_ATLAS_CLOUD_EXTERNAL_URL}/v2/${TEST_ATLAS_CLOUD_EXTERNAL_GROUP_ID}#/explorer`
);
} else {
await browser.navigateTo('http://localhost:7777/');
}

const compass = new Compass(name, browser, {
mode: 'web',
writeCoverage: false,
Expand Down
155 changes: 83 additions & 72 deletions packages/compass-e2e-tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
LOG_PATH,
removeUserDataDir,
updateMongoDBServerInfo,
TEST_ATLAS_CLOUD_EXTERNAL_URL,
} 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 (!TEST_ATLAS_CLOUD_EXTERNAL_URL) {
// 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 Down Expand Up @@ -139,34 +142,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 (!TEST_ATLAS_CLOUD_EXTERNAL_URL) {
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 +263,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 +277,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

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
Loading