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): port most e2e tests to multiple connections COMPASS-7925 COMPASS-8001 #5878

Merged
merged 50 commits into from
Jun 24, 2024

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jun 7, 2024

Adds support for --test-multiple-connections, ports most tests, disables a bunch of others (with tons of new TODO comments) and adds a temporary new evergreen build variant for running the multiple connections e2e tests.

@lerouxb lerouxb added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Jun 7, 2024
@@ -87,7 +87,7 @@ export function NavigationItem({
if (item.type === 'connection') {
return {
'data-connection-id': item.connectionInfo.id,
'data-connection-name': item.connectionInfo.favorite?.name,
'data-connection-name': item.name,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some connections don't have a favourite name. Not sure if that's intended. But there's always a name because that's what's displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea that's a good catch - name will always be there

@@ -166,7 +166,7 @@ export function ConnectionFormModalActions({

<div className={saveAndConnectStyles}>
<Button
data-testid="save-and-connect-button"
data-testid="save-button"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had two buttons with the same identical save-and-connect-button testid.

@@ -29,7 +29,7 @@ export function SidebarHeader({
onAction(actionName: Action): void;
}): React.ReactElement {
return (
<div className={sidebarHeaderStyles}>
<div className={sidebarHeaderStyles} data-testid="sidebar-header">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

testid used in e2e selectors for some custom browser commands that need to go click somewhere/anywhere that will have no effect other than that it will blur whatever is actually focused and give some browser.waitUntil() a chance to succeed next time.

Total hack. Documented in the custom commands but that doesn't necessarily show up in the diff in this PR so I figured I'd just leave this comment here.

.evergreen/buildvariants.yml Outdated Show resolved Hide resolved
@lerouxb lerouxb changed the title chore(e2e): port most e2e tests to multiple connections COMPASS-7925 chore(e2e): port most e2e tests to multiple connections COMPASS-7925 COMPASS-8001 Jun 12, 2024
@lerouxb lerouxb marked this pull request as ready for review June 12, 2024 10:43
): Promise<string> {
if (TEST_MULTIPLE_CONNECTIONS && color === 'color1') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a total hack for now, but this whole command should just go away when we remove single connections.

import type { CompassBrowser } from '../compass-browser';
import * as Selectors from '../selectors';
import { expect } from 'chai';

export async function saveFavorite(
browser: CompassBrowser,
favoriteName: string,
color: `color${number}`
// TODO(COMPASS-8003): make color optional for multiple connections
color: `color${number}` | string
Copy link
Contributor Author

@lerouxb lerouxb Jun 20, 2024

Choose a reason for hiding this comment

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

Same here: This whole command will also just go away along with single connections because you can now save a favorite via the connection form with setConnectioFormState().

color9: 'Purple',
};

function colorValueToName(color: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's probably a better way to do this, but this is so we can select an option by its display value. ie. the select's value is 'color1', but the option is displayed as 'Red'. We can probably import this from somewhere rather than hardcoding it..

Copy link
Contributor

Choose a reason for hiding this comment

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

yea this is available in connection-form package most likely, maybe not exported

@@ -18,6 +18,8 @@ export async function waitForConnectionResult(
// indicator that we are connected to the server
selector = TEST_COMPASS_WEB
? '[data-testid="workspace-tab-button"][title=Databases]'
: TEST_MULTIPLE_CONNECTIONS
? Selectors.SidebarTreeItems
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This kinda assumes that any connection is good enough, but you could have already had a connection before you even connected, so then it actually won't wait for anything. Ideally we'd wait for the element for the expected connection name to appear, but we don't have that context here.

);
let actual = '';
await browser.waitUntil(
async () => {
return (actual = await clipboard.read()) === expected;
actual = await clipboard.read();
console.log({ actual, 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.

Yes this is intentional.

Copy link
Contributor

@himanshusinghs himanshusinghs left a comment

Choose a reason for hiding this comment

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

Its looking good. I also think we unblocked a few todos like import export connections, protectConnectionString flake.

@@ -87,7 +87,7 @@ export function NavigationItem({
if (item.type === 'connection') {
return {
'data-connection-id': item.connectionInfo.id,
'data-connection-name': item.connectionInfo.favorite?.name,
'data-connection-name': item.name,
Copy link
Contributor

Choose a reason for hiding this comment

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

yea that's a good catch - name will always be there

color9: 'Purple',
};

function colorValueToName(color: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

yea this is available in connection-form package most likely, maybe not exported

@@ -1605,7 +1606,7 @@ describe('Collection aggregations tab', function () {
);

await browser.selectOption(
Selectors.AggregationWizardSortFormDirectionSelect(0),
`${Selectors.AggregationWizardSortFormDirectionSelect(0)} button`,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think i saw somewhere button removed from dropdown selector - was wondering if that's more hit and try thingy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no recollection of it. I think I moved it around while in Berlin but can't remember if it was really required or not. I think the reasoning is that the selector points to the component and the button is a thing inside there and therefore by not making it point at the button the selector is more reusable.

All I know is that this has been rock solid ever since, so probably fine ;)

Comment on lines +56 to +59
// TODO(COMPASS-8029): This regressed for multiple connections
if (TEST_MULTIPLE_CONNECTIONS) {
this.skip();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be now fixed with #5955

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'm still leaving that for a follow-up task and PR, though ;)

@lerouxb lerouxb merged commit 27e0721 into main Jun 24, 2024
30 checks passed
@lerouxb lerouxb deleted the port-multiple-connections-e2e branch June 24, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature flagged PRs labeled with this label will not be included in the release notes of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants