Looked into return code for nuget download and imporved logging #6943
Conversation
|
|
||
| try { | ||
| tl.publishTelemetry(area, feature, Object.assign(getDefaultProps(), properties)); | ||
| } catch (err) { |
There was a problem hiding this comment.
this is not required as it's already handled in PublishTelemetry call
There was a problem hiding this comment.
and if it is not handled this needs to go into that code..
In reply to: 180741561 [](ancestors = 180741561)
There was a problem hiding this comment.
Just wanted to be double sure on our end and not rely on the try catch there in case someone changed it without our notice.
Can remove it if you guys feel its not required. I personally thought its safer to leave it around just for the simple fact that the telemetry implementation may change.
There was a problem hiding this comment.
removed this from here
| @@ -194,7 +198,12 @@ async function acquireAndCacheVsTestPlatformNuget(testPlatformVersion: string): | |||
|
|
|||
| tl.debug(`Downloading Test Platform version ${testPlatformVersion} from ${packageSource} to ${downloadPath}.`); | |||
| let startTime = perf(); | |||
| await nugetTool.exec(); | |||
| const resultCode = await nugetTool.exec(); | |||
There was a problem hiding this comment.
Remove nested if blocks and invert the conditions
| executionStartTime = perf(); | ||
|
|
||
| tl.setResourcePath(path.join(__dirname, 'task.json')); | ||
|
|
||
| console.log(tl.loc('StartingInstaller')); | ||
| console.log('=============================================================================='); |
There was a problem hiding this comment.
can this move up as the first one..? even before the call to os.platform ?
There was a problem hiding this comment.
So we need to load the resource file before i can shoot any localized messages to the console.
For the diagnostic version of the task we are going to upload i was just simply thinking of adding a tl.debug(i++); before every statement in the script.
There was a problem hiding this comment.
requires localization to output this
|
Had not added try catch around the startInstaller function as none of the other tasks i saw nested the first call in a try catch block. Was just maintaining parity by adding the try catch inside the function. Can add this if needed but i don't think its required. |
ShreyasRmsft
deleted the
users/ShreyasRMsft/vstestToolInstallerImprovements
branch
Looked into return code for nuget download and imporved logging