Skip to content

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Jan 18, 2024

Skipping everything but the "time to first query" tests, left lots of TODOs.

(process.env.MONGODB_VERSION?.endsWith('-enterprise') && 'yes') ?? 'no';

// should we test compass-web (true) or compass electron (false)?
export const TEST_COMPASS_WEB = process.argv.includes('--test-compass-web');
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 don't know what to call this thing. We can easily find/replace it with a better name. Not sure if it is best to calculate it based off an env var or a cli parameter. We need to use this both in mocha tests and in helpers and having it as a const you can import seems like the cleanest of the options I can think of.

browser: CompassBrowser
): Promise<void> {
const connectScreenElement = await browser.$(Selectors.ConnectSection);
// TODO: can we not just make this the same selector in both cases somehow?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, we might be able to use connection-form in the sandbox, this should make this stuff more unified even if connection form will not end up in data explorer

): Promise<void> {
const tab = browser.$(Selectors.collectionSubTab(tabName));
const selectedTab = browser.$(Selectors.collectionSubTab(tabName, true));
const tab = Selectors.collectionSubTab(tabName);
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 have a lot of cases of this bug. in this case tab is a selector (ie. a string) and browser.clickVisible(selector) takes a selector, not an element. And in quite a few places we mistakenly sent it either an element or a promise that will resolve to an element. I'm wondering if we can't just make browser.clickVisible() take either a selector OR an element. OR better yet maybe we can have element custom commands and not just browser ones.

const sidebarNavigationItem = browser.$(
Selectors.sidebarInstanceNavigationItem(tabName)
);
const sidebarNavigationItem =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again - sidebarNavigationItem was a promise that would resolve to an element which is probably not what we want to pass to clickVisible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is specifically not awaited so that clickVisible can re-request element from a selector instead of trying to click an element that was potentially removed / reinserted in DOM. Like an element matching .foo is in the DOM, but awaiting here would keep a reference to a specific DOM node that is gone already. In webdriver types this is referred to as ChainablePromiseElement. Awaiting a specific element and passing it around was causing issues in a few places, so this is why clickVisible was extended to support this. If it's not an issue anymore, feel free to remove it, but then you'd probably want to remove the ability to pass ChainablePromiseElement to the method itself too

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 relented and just added ChainalePromiseElement support everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw this is usually why I just pass a selector and would re-select based on the selector. Otherwise you inevitably end up with a stale element eventually. I'm not 100% sure if ChainablePromiseElement won't end up caching the element id internally somehow anyway.

Also we have sooo many webdriver warnings flying past about stale elements in version 8 now.. 😬

Selectors.DuplicateViewModalConfirmButton
);
confirmDuplicateButton.waitForEnabled();
await confirmDuplicateButton.waitForEnabled();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something about webdriverio's old types caused typescript to not notice that this is a promise that wasn't being awaited. Now it works as expected with the latest webdriver.

capabilities: {
browserName: 'chrome',
browserName: 'chromium',
browserVersion: '120.0.6099.216', // TODO: this must always be the corresponding chromium version for the electron version
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 sucks. We have to set this otherwise webdriver now goes down a path where it tries to use the electron version as the chromium version and then gets the chrome driver version for that version.. Electron always includes the chromium version in their release notes. Maybe there's a way to programmatically get this? https://releases.electronjs.org/release/v28.1.4

Copy link
Collaborator

Choose a reason for hiding this comment

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

$ echo 'console.log("CHROME_VERSION=" + process.versions.chrome)' | npx electron -i 2>&- | grep -o 'CHROME_VERSION=.*'
CHROME_VERSION=120.0.6099.216

could probably adapt this into a JS script fairly easily

await browser.waitUntil(async () => {
const element = await browser.$(selector);
await element.waitForDisplayed();
await element.click(); // focus
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like who knows how many subtle things like this broke in the webdriverio upgrade. await element.clearValue() now does nothing and await element.setValue() appends to the text that's already in the form field's value...

@lerouxb lerouxb changed the title Compass web e2e [PoC] [DO NOT MERGE] Compass web e2e [PoC] WRITING-15921 Feb 7, 2024
@lerouxb lerouxb closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants