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

e2e test: update db connections test #5454

Merged
merged 12 commits into from
Nov 22, 2024
Merged

e2e test: update db connections test #5454

merged 12 commits into from
Nov 22, 2024

Conversation

midleman
Copy link
Contributor

@midleman midleman commented Nov 22, 2024

Intent

Update the existing database connections test to support the new DB connections pane changes by toggling the appropriate feature flag (which restarts the app) and updating the test logic accordingly.

Approach

Modify the database connections test to use the new fixture and update all locators and methods to interact with the updated connections pane.

QA Notes

You might be wondering what those test.steps are there for? They make the HTML report easier to read like so:

Screenshot 2024-11-22 at 9 21 06 AM

If you forget what it looks before, it was this nonsense:
Screenshot 2024-11-22 at 10 07 29 AM

I think this will be a little bit of an art. They are optional and you certainly don't want them for every single line of code, but it does help lump steps together to ease debugging of reports. Note: the trace viewer will not show them grouped like the report does.

@midleman midleman changed the title Mi/db connections test e2e test: update db connections test Nov 22, 2024
@midleman midleman marked this pull request as ready for review November 22, 2024 16:48
this.deleteConnectionButton = code.driver.page.getByLabel('Delete Connection');
this.disconnectButton = code.driver.page.getByLabel('Disconnect');
this.connectIcon = code.driver.page.locator('.codicon-arrow-circle-right');
this.connectionItems = code.driver.page.locator('.connections-list-item');
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but I think Nick prefers just putting these outside the constructor since they are static.

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 could do that, but they are dependent on this.code.driver.page since they are Locators and not just a string. which means I won't be able to use them like this in the test:
await app.workbench.positronConnections.disconnectButton.click();

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought locators were just static pointers to what would be used to find an element once they are used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its really a nitpick... just keeping you in the loop regarding what Nick had mentioned perfering.

testlabauto
testlabauto previously approved these changes Nov 22, 2024
Copy link
Contributor

@testlabauto testlabauto left a comment

Choose a reason for hiding this comment

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

LGTM!

@midleman midleman merged commit bae3b68 into main Nov 22, 2024
4 checks passed
@midleman midleman deleted the mi/db-connections-test branch November 22, 2024 17:32
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants