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

Refactor smoke UI automation into separate package #80293

Merged
merged 8 commits into from
Sep 10, 2019

Conversation

jasongin
Copy link
Member

@jasongin jasongin commented Sep 4, 2019

This change divides the smoke project into two packages:

  • The existing test/smoke package now contains only the actual test cases plus the code for setting up and running those tests via mocha.
  • A new package at test/automation contains the UI automation "driver" and per-component automation modules. The smoke package depends on this one via a local filesystem reference.

Besides enforcing a clearer separation between those two parts, this also makes it possible to build a reusable npm package for VS Code UI automation. VS Code might never support broad consumption of this package, and there is no immediate plan to publish it to the global npm registry. My goal is to make it easier for some first-party extensions (VS Live Share and others) to run UI tests in VS Code, and consume updates to the test automation package when testing updated releases of VS Code. There is no implication or expectation that the automation package or any part of the VS Code DOM would avoid breaking changes or follow semver rules. But by re-using the automation modules we can reduce the amount of effort required to adapt to the changes.

I verified the existing VS Code smoke tests are still passing with these changes. The process for building the tests is slightly different due to having two packages where there was one; I have tried to make it as simple as possible, and I updated the smoke README. Let me know if you'd like something done differently.

@joaomoreno @Tyriar

@joaomoreno joaomoreno self-assigned this Sep 4, 2019
@joaomoreno joaomoreno self-requested a review September 4, 2019 06:07
@joaomoreno joaomoreno added this to the September 2019 milestone Sep 4, 2019
@Tyriar
Copy link
Member

Tyriar commented Sep 4, 2019

FYI @DonJayamanne @SounD120

@Tyriar Tyriar self-assigned this Sep 4, 2019
@Tyriar Tyriar self-requested a review September 4, 2019 16:59
@@ -3,13 +3,10 @@
"version": "0.1.0",
"main": "./src/main.js",
"scripts": {
"preinstall": "yarn --cwd ../automation",
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to have "precompile": "yarn --cwd ../automation compile"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I intended this to install packages there also. (If packages are already installed, yarn is very quick to skip that step.) Otherwise you would have to go manually do a yarn install in the automation directory first.

Copy link
Member

Choose a reason for hiding this comment

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

When you run yarn in the root folder it will run for a bunch of sub-folders, can we not hook into that same mechanism? It's being done for test/smoke currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll look into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I added yarnInstall('test/automation') to the root postinstall.js script. And in this package.json I removed the preinstall script and updated the compile script to also compile the automation package. I think that simplifies the workflow when you might be changing both test/automation and test/smoke at the same time.

test/smoke/package.json Show resolved Hide resolved
@joaomoreno joaomoreno removed their request for review September 5, 2019 07:37
@joaomoreno
Copy link
Member

@Tyriar, since you've been deep into this lately, do you mind taking this?

@Tyriar
Copy link
Member

Tyriar commented Sep 5, 2019

@joaomoreno sure, I already looked over the whole thing and it looks pretty good, just those comments above and testing it myself left.

Tyriar
Tyriar previously requested changes Sep 6, 2019
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Watch now doesn't work quite right because of how tsc rewrites to the terminal, you'll want to use the --preserveWatchOutput flag to avoid this

@Tyriar Tyriar dismissed their stale review September 6, 2019 16:11

I did this

@Tyriar
Copy link
Member

Tyriar commented Sep 6, 2019

Just the yarn thing remaining, then good to merge. Anything needed after this for your team to start using it, or is that it?

@jasongin
Copy link
Member Author

jasongin commented Sep 6, 2019

We don't need anything else right now. We've already been using this automation library for nearly a year now to test VS Live Share. It was just a tedious manual process to extract from the vscode repo in a usuable form. Now that will be simple, so we can take updates more frequently.

I had planned to contribute some small changes to the automation library to make multi-window automation easier. For Live Share we automate scenarios with host in one window and guest in another. Currently we have to access some private members of the Code class to make that work. I'll make a separate PR for that.

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for finishing up the yarn stuff 👍

@Tyriar Tyriar merged commit 7bb6df9 into microsoft:master Sep 10, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
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.

3 participants