Skip to content

Missed a couple mocked commands. Bump patch version #334

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 2, 2018
Merged

Missed a couple mocked commands. Bump patch version #334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b8c2506
Missed a couple commands. Catch unmocked commands at compile time. …
brcrista Mar 26, 2018
17e5815
Merge branch 'users/brcrista/mock-answers2'
brcrista Mar 26, 2018
53185b5
Need to bump again, had un-synced changes upstream
brcrista Mar 26, 2018
c80f45c
Strings should be single-quoted
brcrista Mar 26, 2018
5965041
Bump minor version
brcrista Mar 26, 2018
11c1482
clarify TODO
brcrista Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions node/mock-answer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import path = require('path');
import fs = require('fs');
import * as path from 'path';
Copy link
Contributor Author

@brcrista brcrista Mar 26, 2018

Choose a reason for hiding this comment

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

import ... require has slightly different semantics than import ... from. My understanding is that require executes the module as the CommonJS and AMD require function does so that any side effects from the module are applied. import ... from is the normal way to import a module in TypeScript.

import * as fs from 'fs';

export interface TaskLibAnswerExecResult {
code: number,
Expand All @@ -15,10 +15,25 @@ export interface TaskLibAnswers {
find?: { [key: string]: string[] },
findMatch?: { [key: string]: string[] },
ls?: { [key: string]: string },
osType?: { [key: string]: string },
rmRF?: { [key: string]: { success: boolean } },
which?: { [key: string]: string; },
stats?: { [key: string]: any }, // Can't use `fs.Stats` as most existing uses don't mock all required properties
which?: { [key: string]: string },
}

// TODO TypeScript 2.1: replace with `keyof TaskLibAnswers`
export type MockedCommand = 'checkPath'
| 'cwd'
| 'exec'
| 'exist'
| 'find'
| 'findMatch'
| 'ls'
| 'osType'
| 'rmRF'
| 'stats'
| 'which';

export class MockAnswers {
private _answers: TaskLibAnswers;

Expand All @@ -29,14 +44,14 @@ export class MockAnswers {
this._answers = answers;
}

public getResponse(cmd: string, key: string, debug: (message: string) => void): any {
public getResponse(cmd: MockedCommand, key: string, debug: (message: string) => void): any {
Copy link
Member

@stephenmichaelf stephenmichaelf Mar 26, 2018

Choose a reason for hiding this comment

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

Breaking change? May want to bump at least the minor since it's mock and not the main code.

Copy link
Member

@stephenmichaelf stephenmichaelf Mar 26, 2018

Choose a reason for hiding this comment

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

Maybe MockCommand?

Copy link
Contributor Author

@brcrista brcrista Mar 26, 2018

Choose a reason for hiding this comment

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

Yeah, it's technically a breaking change if people are calling getResponse directly for some reason. There aren't any uses right now in vsts-tasks -- it's just used internally in mock-task and mock-toolrunner.

I said MockedCommand since it's "one of the commands that are mocked" and not the mock itself.

Copy link
Contributor

@madhurig madhurig Mar 26, 2018

Choose a reason for hiding this comment

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

There might be 3rd party extensions using it

Copy link
Contributor Author

@brcrista brcrista Mar 26, 2018

Choose a reason for hiding this comment

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

Yeah, we can bump the minor version

debug(`looking up mock answers for ${JSON.stringify(cmd)}, key '${JSON.stringify(key)}'`);
if (!this._answers) {
throw new Error('Must initialize');
}

if (!this._answers[cmd]) {
debug(`no mock responses registered for given cmd`);
debug(`no mock responses registered for ${JSON.stringify(cmd)}`);
return null;
}

Expand Down
2 changes: 1 addition & 1 deletion node/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "vsts-task-lib",
"version": "2.3.1",
"version": "2.4.0",
"description": "VSTS Task SDK",
"main": "./task.js",
"typings": "./task.d.ts",
Expand Down