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

detect vstest.console.exe15 #3158

Merged
merged 7 commits into from
Dec 13, 2016
Merged

detect vstest.console.exe15 #3158

merged 7 commits into from
Dec 13, 2016

Conversation

mutn3ja
Copy link
Contributor

@mutn3ja mutn3ja commented Nov 28, 2016

  • able to detect vstest.console.exe15
  • end to end validation done
    • Machine with both VS2015 and VS2017 ('latest' selected in build definition)
    • Machine with VS2015 only ('latest' selected in build definition)
    • Machine with VS2017 only ('latest' selected in build definition)
    • Machine with VS2017 only ('specify location' mode)

let powershellTool = tl.tool('powershell');
let powershellArgs = ['-file', path.join(__dirname, 'vs15Helper.ps1')]
powershellTool.arg(powershellArgs);
let xml = powershellTool.execSync().stdout;
Copy link
Contributor

Choose a reason for hiding this comment

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

be aware that execSync will not get live output and it will buffer the output. Convenient for running quick clis.

Consider await tool exec.

@mutn3ja mutn3ja assigned mutn3ja and nigurr and unassigned mutn3ja Dec 5, 2016

function locateVSVersion(): Q.Promise<[number, string]> {
var defer = Q.defer<[number, string]>();
let vsVersion: number = parseFloat(vsTestVersion);
Copy link

Choose a reason for hiding this comment

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

vsTestVersion [](start = 39, length = 13)

pass this as argument. no global variable usage


function locateVSVersion(): Q.Promise<[number, string]> {
var defer = Q.defer<[number, string]>();
let vsVersion: number = parseFloat(vsTestVersion);
Copy link

Choose a reason for hiding this comment

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

let

return deferred.promise;
}

function getVSTestConsole15Path(): Q.Promise<string> {
Copy link

Choose a reason for hiding this comment

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

getVSTestConsole15Path [](start = 9, length = 22)

use return on Q call why to use unnecessary defer variables

powershellTool.arg(powershellArgs);
let xml = powershellTool.execSync().stdout;
let deferred = Q.defer<string>();
Q.nfcall(xml2js.parseString, xml).then((data) => {
Copy link

Choose a reason for hiding this comment

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

and why it has to be async call?

}


function locateVSVersion(): Q.Promise<[number, string]> {
Copy link

Choose a reason for hiding this comment

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

why not create VS version object something like that and use it instead of map

}


function locateVSVersion(): Q.Promise<[number, string]> {
Copy link

Choose a reason for hiding this comment

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

i dont see reason for these calls to be async

let selectedVersion = versions[versions.length - 1];
deferred.resolve([selectedVersion, getVSTestLocation(selectedVersion)]);
return deferred.promise;
}
Copy link

Choose a reason for hiding this comment

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

aren't you supposed to resolve something here? is this works?

Write-Host ([System.Management.Automation.PSSerializer]::Serialize($instance.Path))
Copy link

Choose a reason for hiding this comment

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

can this be an empty or null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Serialize works properly in case of null or empty.

@@ -20,6 +20,15 @@ const TITestSettingsNameTag = "testSettings-5d76a195-1e43-4b90-a6ce-4ec3de87ed25
const TITestSettingsIDTag = "5d76a195-1e43-4b90-a6ce-4ec3de87ed25";
const TITestSettingsXmlnsTag = "http://microsoft.com/schemas/VisualStudio/TeamTest/2010"

class VSTestConsoleInfo {
Copy link

Choose a reason for hiding this comment

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

interface

vstestLocation = vstest[1];
locateVSVersion(vsTestVersion)
.then(function(vsTestConsoleInfo) {
let vsVersion = vsTestConsoleInfo.version;
Copy link

Choose a reason for hiding this comment

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

why do you need intermediate vars

try {
let vs15InstallDir = result['Objs']['S'][0];
vstestconsolePath = path.join(vs15InstallDir, 'Common7', 'IDE', 'CommonExtensions', 'Microsoft', 'TestWindow', 'vstest.console.exe');
} catch (e) {
Copy link

Choose a reason for hiding this comment

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

dont you want to log what's the exception?

var defer = Q.defer<[number, string]>();
let vsVersion: number = parseFloat(vsTestVersion);
function locateVSVersion(version: string): Q.Promise<VSTestConsoleInfo> {
let deferred = Q.defer<VSTestConsoleInfo>();
Copy link

Choose a reason for hiding this comment

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

get rid of these deferred vars. your promises can be chained, can be returned directly. code is cleaner and simpler

@nigurr
Copy link

nigurr commented Dec 12, 2016

What is the testing done? list down test cases. LGTM

@mutn3ja mutn3ja merged commit 9ada20e into master Dec 13, 2016
@bryanmacfarlane bryanmacfarlane deleted the users/prmutn/tsDetectVSWillow branch January 31, 2018 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants