Skip to content

Conversation

@mattseddon
Copy link
Contributor

@mattseddon mattseddon commented Jul 8, 2022

1/2 main <- this <- #2013

Closes #1471.

Please review the approach. Not much has changed from the last PR (#1993). We just have to dvc pull the required data and ensure we checkout the entire git history to get things working in the CI.

Comments inline.

There is one remaining test that I would like to write. I'll produce a follow up PR.

@mattseddon mattseddon marked this pull request as ready for review July 8, 2022 10:56
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

Ideally we would add this to the Continuous Integration CI, it's just a question of how liberal we want to be with our GitHub Actions minutes. We could add a few conditions to save minutes, maybe something like skipping the e2e run on draft PRs?

I'm also curious how much time we could save on e2e tests by stripping down the dvc project that drives it to something that only layers the bare minimum above dvc, removing even torch in favor of something like static fixture data; what we lose in authenticity would easily be made up for by how huge it would be to be able to run e2e tests as liberally as our other suites.

A hyper-minimal testbed project wouldn't necessarily have to replace our current demo, but there could also arguments that it should.

As far as cross-platform tests go, that's a whole other question. Considering windows and mac runners consume 2 and 10 times as much minutes as linux runners, I think our best bet is to save cross-platform e2e runs for situations when they're specifically requested through either workflow-dispatch or some more convenient form of trigger like a special label.

@mattseddon
Copy link
Contributor Author

I am going to add to the Continuous Integration workflow and strip the macOS-latest from the matrix.os property in .github/workflows/cross-platform-test.yml.

For me, having a realistic project is important as it is an end to end test. It is also important to use something realistic when we are manually testing the project.

strategy:
matrix:
os: [macOS-latest, windows-latest]
os: [windows-latest]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[F] By removing the less valuable macOS-latest tests we make get back more minutes than we expend running the E2E tests in the main workflow.

import { join } from 'path'
import { getProcessPlatform } from '../env'

export const getVenvBinPath = (cwd: string, envDir: string, name: 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.

[F] This needs to be separated so we don't have to require 'vscode'

await workbench.executeCommand('DVC: Show Experiments')

await webview.open()
await webview.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.

[F] These seem like better names to me because the methods don't actually open/close the webview

expandRowButton: 'button[title="Expand Row"]',
row: '[role=row]',
table: '[role=table]'
table: '[role=tree]'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[F] This changed when we move up to v18 of React

await browser.switchToFrame(null)
await browser.switchToFrame(null)
},
afterTest: async (test, __, { passed }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[F] This is close to what is provided in the example tests for the service. If I get time I will extend the way that we do timeouts to also take screenshots.

@mattseddon mattseddon enabled auto-merge (squash) July 12, 2022 23:37
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 01e59a9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.6% (0.0% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 159f45c into main Jul 12, 2022
@mattseddon mattseddon deleted the run-e2e branch July 12, 2022 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use wdio-vscode-service for e2e tests

3 participants