Expand test coverage by smashwilson · Pull Request #1169 · microsoft/vscode-pull-request-github

smashwilson · 2019-05-22T13:29:32Z

Let's expand the scope of our test suite to cover more of the extension's functionality. I'm starting here to provide us with some immediate benefits:

It'll enable myself, @kuychaco, and @vanessayuenn to make changes with a lower risk of causing regressions. We're new to the codebase and don't yet have a mature mental model of how the source is structured and interconnected, so we're likely to break unrelated functionality otherwise.
I'll have a chance to learn how things work and tour the code myself in a way that's more active than just reading it.
We'll be better equipped to evaluate and accept community contributions with confidence.
It'll be easier to update dependencies and enable dependabot.
We'll become less reliant on manual testing checklist issues.

Backfitting tests to existing code is tedious and time-consuming. My goal here isn't to get us to full coverage - mostly, I'm concerned with introducing a skeleton structure that lowers the barriers to adding tests organically as we work in the future. I'll call this PR "complete" once I have a test suite in place for a set of basis cases that cover the mocking needs of different parts of the codebase.

I'm also doing my best to minimize structural changes to the source. I'll likely need to change some things to make things more easily testable, but I'll endeavor to keep them small and scopes to keep this effort tractable and minimize risk.

Remaining

Modules that use the VSCode API. The primary concern here is being able to avoid collisions with globally-scoped resources like command IDs.

src/commands.ts
src/github/credentials.ts
src/github/pullRequestManager.ts

Modules that expect to use the network. Testing these effectively requires a way to succinctly mock GraphQL and REST responses.

src/github/githubRepository.ts
src/github/pullRequestOverview.ts

Modules that use git operations. These rely on a way to mock git commands performed through the git extension's exported API.

src/github/pullRequestGitHelper.ts

Source that defines React components loaded within a Webview.

preview-src/app.tsx

Integration tests that exercise the UI components contributed to VSCode, like the tree view and the status bar tiles.

src/view/prsTreeDataProvider.ts

Finally - general support infrastructure work:

Support *.gql imports in Mocha tests.
Document the Builder setup.
Deal with the extension under test attempting to register commands in the MockCommandRegistry during initialization

msftclas · 2019-05-22T13:29:48Z

All CLA requirements met.

In retrospect, I'm not sure why I expected that to work.

CredentialStore instances need to be disposed after use to prevent the command registration in its contructor from colliding.

rebornix · 2019-05-23T03:43:05Z

@smashwilson this is a really great effort!

When we introduced graphql into the project, we separate models and views by putting everything view related into src/view and others into their own components common, github.

For testing src/view, the only thing we need to mock (if we didn't leak other layers into it recently) is PullRequestModel and PullRequestManager. IMO we have unit tests for PullRequestModel and PullRequestManager and for src/view, we do integration tests directly by VS Code test runner.

…hub into aw/expand-tests

TypeScript 2.x isn't smart enough to follow some of the conditional types in the Builder work I'm preparing.

I needed to enable these to import lodash. Unsure why and open to pushback 😉

smashwilson

Self-review to provide a little more context 😄

smashwilson · 2019-06-04T16:45:08Z

.vscode/settings.json

 	"files.trimTrailingWhitespace": true,
-	"editor.insertSpaces": false
+	"editor.insertSpaces": false,
+	"typescript.tsdk": "node_modules/typescript/lib"


It looks like this prompts VS Code to make the version of TypeScript from our own node_modules/ available as a language server in addition to the bundled one. I think this was added automatically when I toggled it in my workspace?

I would revert this, I don't think we need this setting to be on for everyone developing this project

smashwilson · 2019-06-04T16:50:37Z

package.json

    "@types/graphql": "^14.0.5",
    "@types/keytar": "^4.0.1",
    "@types/lodash": "^4.14.106",
+    "@types/lodash.isequal": "4.5.5",


I'm not sure why we depend on @types/lodash but not lodash itself?

I believe @kuychaco found a place where we were requiring lodash directly even though it's a transitive dependency and promoted it to a direct dependency instead. We should be able to replace the places I used lodash.isequal with lodash once that's merged in.

smashwilson · 2019-06-04T16:52:27Z

package.json

    "tslint-webpack-plugin": "^1.2.2",
-    "typescript": "^2.1.4",
+    "tty": "1.0.1",
+    "typescript": "3.4.5",


I needed to upgrade TypeScript because 2.x wasn't able to follow some of the type inference shenanigans used by the builders.

smashwilson · 2019-06-04T16:54:29Z

preview-src/tsconfig.json

+			"dom",
+			"esnext.asynciterable",
+			"es2017"
+		],


For consistency with the root tsconfig.json settings. This was necessary to successfully run tsc on preview-src/ outside of webpack, which I needed to do in order to run tests here.

I'm not sure why this tsconfig has a different target value than the root one, though? I feel like we should be able to have this one inherit from the other to stay consistent.

smashwilson · 2019-06-04T16:56:36Z

src/common/utils.ts

 import { Event, Disposable } from 'vscode';
 import { sep } from 'path';
-import * as moment from 'moment';
+import moment = require('moment');


It took me a while to figure out the difference between import moment from 'moment', import * as moment from 'moment', and import moment = require('moment').

I believe this one's correct here (and the other places I've updated it) because of this section from the TypeScript docs:

When exporting a module using export =, TypeScript-specific import module = require("module") must be used to import the module.

smashwilson · 2019-06-04T17:01:41Z

src/github/pullRequestManager.ts

+
+	createGitHubRepository(remote: Remote, credentialStore: CredentialStore): GitHubRepository {
+		return new GitHubRepository(remote, credentialStore);
+	}


I believe sinon has the ability to mock a constructor, but it always feels too invasive for my tastes. (What if there's another instantiation somewhere on the stack that I don't want to mock?)

smashwilson · 2019-06-04T17:02:38Z

src/github/pullRequestOverview.ts

 				}
 			});
 		}).catch(e => {
+			console.error(e.stack);


Oops. I'll delete this one.

smashwilson · 2019-06-04T17:03:17Z

src/test/builders.test.ts

+	aChildProp: {linked: ChildBuilder},
+});
+
+describe('Builders', function () {


Fun fact: almost all of the actual complexity of the Builder tests is in getting them to typecheck. 🌠

smashwilson · 2019-06-04T17:05:43Z

src/test/index.ts

+	});
+
+	await addTests(mocha, testsRoot);
+	await addTests(mocha, path.resolve(testsRoot, '../../preview-src/dist/preview-src/test'));


This needs to be a custom runner primarily so that we can add tests from preview-src/. It's also a convenient place to install the MockWebviewEnvironment.

smashwilson · 2019-06-04T17:08:23Z

test.sh

+if [ ! -f media/extension.js ]; then
+	printf "Running webpack\n"
+	webpack --env.development
+fi


☝️ This bit will be useful when we run in CI.

Fun fact: the tests are loaded from the tsc-compiled JavaScript emitted by this script, but the development extension that's loaded into the extension test host uses media/extension.ts, so they both need to exist.

smashwilson · 2019-06-05T14:10:06Z

Okay, merged in the changes from #1173 and got everything compiling and green again. Ready for review for reals this time 😄

…hub into aw/expand-tests

Co-Authored-By: <smashwilson@github.com>

RMacfarlane

Sorry, I missed the bus in reviewing this!

RMacfarlane · 2019-06-07T18:44:48Z

.vscode/settings.json

 	"files.trimTrailingWhitespace": true,
-	"editor.insertSpaces": false
+	"editor.insertSpaces": false,
+	"typescript.tsdk": "node_modules/typescript/lib"


I would revert this, I don't think we need this setting to be on for everyone developing this project

RMacfarlane · 2019-06-07T18:46:39Z

package.json

  },
  "dependencies": {
    "@octokit/rest": "^16.21.0",
+    "@types/glob": "7.1.1",


this should be a dev dependency, right

RMacfarlane · 2019-06-07T22:22:55Z

src/github/credentials.ts

 *--------------------------------------------------------------------------------------------*/

-import * as Octokit from '@octokit/rest';
+import Octokit = require('@octokit/rest');


For octokit, the module ships with a .d.ts typings file, so I think the normal import syntax is fine

smashwilson added 5 commits May 22, 2019 09:19

Sinon for mocking, temp for temp dirs

6d5a8f8

Test mocks

359e777

Turn CredentialStore and PullRequestManager into Disposables

bc9bd3b

Register the PullRequestManager as a disposable

5e5f89d

Infrastructure and setup for command tests

989d45d

smashwilson changed the title ~~Infrastructure and setup for command tests~~ Expand test coverage May 22, 2019

smashwilson added 6 commits May 22, 2019 10:08

Remove createSinonSandbox() helper

02ebdfd

In retrospect, I'm not sure why I expected that to work.

Avoid constructing CredentialStore as a global

d00f258

CredentialStore instances need to be disposed after use to prevent the command registration in its contructor from colliding.

A second test for the auth.signout command

4c2286d

Mocha likes function(){} instead of ()=>{}

4ed9943

Split helpers.ts into a mocks/ folder before it gets out of hand

99d8da8

Dispose the PullRequestTreeDataProvider on shutdown

662de43

smashwilson added 16 commits May 23, 2019 09:14

Mock to intercept and isolate command registration

889f235

Typo fix 📝

92fc9c8

Merge branch 'master' of github.com:microsoft/vscode-pull-request-git…

9b796d1

…hub into aw/expand-tests

Let's use builder-pattern to construct test fixtures.

e88a9f0

Template objects for PullRequest-related models

2aeb59b

Take advantage of the PR builder in pullRequestModel tests

3f07225

PullRequestManager test suite

17950d6

CredentialStore suite

17f18f5

GitHubRepository suite

6107404

Rename mock files to be camelCase

ed1405f

Upgrade TypeScript

16e5f94

TypeScript 2.x isn't smart enough to follow some of the conditional types in the Builder work I'm preparing.

Enable esModuleInterop and use node moduleResolution

0d0f52c

I needed to enable these to import lodash. Unsure why and open to pushback 😉

Depend on lodash.isequal

56c8410

Use the package TypeScript version for LSP

637b05b

Correct a few Array types in the GraphQL schema

0a8674b

First cut at a typed Builder implementation

ac9bb41

smashwilson added 11 commits June 3, 2019 10:10

Run webpack if needed

f490ea2

Avoid uglier mocking by making GitHubRepository creation pluggable

d151c04

"Integration"-ish tests for PullRequestsTreeDataProvider

024caf3

createBranch() adds to state.refs too

731d650

Optionally pass CredentialStore in the constructor

c07e85a

Make GitHubRepository creation a nice stubbable method

6c699a6

Return built ManagedPullRequests from addXyzPullRequest methods

908ab07

Nontrivial integration test - expanding the local PRs

6ec562b

🔥 console dot log

46c572f

Document the living hell out of the Builder implementation.

e9871c1

Builder usage documentation

8a627b0

smashwilson commented Jun 4, 2019

View reviewed changes

smashwilson requested review from RMacfarlane, kuychaco, queerviolet and rebornix June 4, 2019 17:10

smashwilson added 5 commits June 5, 2019 08:29

Merge branch 'master' into aw/expand-tests

c7d3241

Adapt builder definitions to match new Octokit types

baf0607

Create the Octokit stub indirectly to use the correct prototype

62083a2

Use the direct lodash dependency

5b9cf25

Octokit method name changed

8cc7186

smashwilson and others added 3 commits June 7, 2019 14:23

Merge branch 'master' of github.com:microsoft/vscode-pull-request-git…

b95d94e

…hub into aw/expand-tests

Actually delete the thing I said I'd delete

aa546ba

Typo.

6b6171e

Co-Authored-By: <smashwilson@github.com>

queerviolet approved these changes Jun 7, 2019

View reviewed changes

smashwilson merged commit 62e2672 into master Jun 7, 2019

smashwilson deleted the aw/expand-tests branch June 7, 2019 19:32

RMacfarlane reviewed Jun 7, 2019

View reviewed changes

smashwilson mentioned this pull request Jun 7, 2019

Back out undesired changes #1224

Merged

Conversation

smashwilson commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining

Uh oh!

msftclas commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rebornix commented May 23, 2019

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smashwilson commented Jun 5, 2019

Uh oh!

RMacfarlane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smashwilson commented May 22, 2019 •

edited

Loading

msftclas commented May 22, 2019 •

edited

Loading