Users/ranjanar/formaster #3643

RanjanarMS · 2017-02-17T10:29:39Z

No description provided.

Users/siddhap/2015vstia

Users/kavipriya/settings

VSTestPlusPlus with UI

Changes for max agents

Porting ps script execution fix

nigurr · 2017-02-17T10:34:08Z

Tasks/VsTest/distributedtest.ts

+import * as ta from './testAgent';
+import * as versionFinder from './versionFinder';
+
+export class DistributedTest {


better naming DistributedTestExecution?

nigurr · 2017-02-17T10:34:48Z

Tasks/VsTest/distributedtest.ts

+        tl.debug('Configure the Agent with DTA... Invoking the createAgent REST API');
+
+        try {
+            const agentId = await ta.TestAgent.createAgent(this.dtaTestConfig.dtaEnvironment, 3);


logging for agentId and process id

nigurr · 2017-02-17T10:36:21Z

Tasks/VsTest/distributedtest.ts

+            try {
+                // TODO: ranjanar: fixasap : This dangerous - Note that the PID only uniquely identifies your child process 
+                // as long as it is running. After it has exited, the PID might have been reused for a different process.
+                process.kill(this.dtaPid);


verify usage of right signal event https://nodejs.org/api/process.html#process_signal_events

remove TODO and check for process name before killing and to eliminate the possibility of killing some random process

nigurr · 2017-02-17T10:37:32Z

Tasks/VsTest/distributedtest.ts

+                // as long as it is running. After it has exited, the PID might have been reused for a different process.
+                process.kill(this.dtaPid);
+            } catch (error) {
+                tl.debug('Process kill failed, pid: ' + this.dtaPid + ' , error :' + error);


shouldn't this be Warning

nigurr · 2017-02-17T10:38:34Z

Tasks/VsTest/distributedtest.ts

+            } catch (error) {
+                tl.debug('Process kill failed, pid: ' + this.dtaPid + ' , error :' + error);
+            }
+            tl.debug(fs.readFileSync(this.dtaTestConfig.dtaEnvironment.dtaHostLogFilePath, 'utf-8'));


doesn't look good 👎
dtaTestConfig.dtaEnvironment.dtaHost

if reading of log fails -> you shouldn't fail the task 🐛

nigurr · 2017-02-17T10:40:03Z

Tasks/VsTest/distributedtest.ts

+    }
+
+    private async startDtaExecutionHost(agentId: any) {
+        tl.rmRF(this.dtaTestConfig.dtaEnvironment.dtaHostLogFilePath, true);


prone to throw ENONET error 🐛

remind me if i miss something, why this is async?

nigurr · 2017-02-17T10:42:10Z

Tasks/VsTest/distributedtest.ts

+    private async startDtaExecutionHost(agentId: any) {
+        tl.rmRF(this.dtaTestConfig.dtaEnvironment.dtaHostLogFilePath, true);
+        const envVars: { [key: string]: string; } = process.env;
+        utils.Helper.addToProcessEnvVars(envVars, 'DTA.AccessToken', this.dtaTestConfig.dtaEnvironment.patToken);


does the method name justify what it's doing 😶

log what you are passing 😞

nigurr · 2017-02-17T10:44:30Z

Tasks/VsTest/distributedtest.ts

+            utils.Helper.addToProcessEnvVars(envVars, 'DTA.TestPlatformVersion', this.dtaTestConfig.vsTestVersion);
+        }
+
+        var exeInfo = await versionFinder.locateTestWindow(this.dtaTestConfig);


pull this up. if you can't find testWindow, don't come down 😒

nigurr · 2017-02-17T10:47:48Z

Tasks/VsTest/distributedtest.ts

+        utils.Helper.addToProcessEnvVars(envVars, 'runsettings', settingsFile);
+        utils.Helper.addToProcessEnvVars(envVars, 'testdroplocation', this.dtaTestConfig.testDropLocation);
+        utils.Helper.addToProcessEnvVars(envVars, 'testrunparams', this.dtaTestConfig.overrideTestrunParameters);
+        utils.Helper.setEnvironmentVariableToString(envVars, 'codecoverageenabled', this.dtaTestConfig.codeCoverageEnabled);


one method doing two things. get as string and send as process variable 😞

nigurr · 2017-02-17T10:48:51Z

Tasks/VsTest/distributedtest.ts

+
+    private async cleanUp(temporarySettingsFile: string) {
+    //cleanup the runsettings file
+    if (temporarySettingsFile && this.dtaTestConfig.settingsFile != temporarySettingsFile) {


indention 👎

nigurr · 2017-02-17T10:50:27Z

Tasks/VsTest/helpers.ts

+import tr = require('vsts-task-lib/toolrunner');
+import path = require('path');
+import Q = require('q');
+import models = require('./models')


use tslint.json and fix simple things ';'

nigurr · 2017-02-17T10:51:56Z

Tasks/VsTest/helpers.ts

+        return obj === null || obj === '' || obj === undefined;
+    }
+
+    public static isNullOrUndefined(obj) {


I checked the use cases. aren't they all supposed to use isNullOrWhitespace and it's very well written previously. no reinvention

nigurr · 2017-02-17T10:52:39Z

Tasks/VsTest/helpers.ts

+
+    public static getXmlContents(filePath: string): Q.Promise<any> {
+        var defer=Q.defer<any>();
+        Helper.readFileContents(filePath, "utf-8")


nigurr · 2017-02-17T10:52:55Z

Tasks/VsTest/helpers.ts

+    }
+
+    public static getXmlContents(filePath: string): Q.Promise<any> {
+        var defer=Q.defer<any>();


indention +100

nigurr · 2017-02-17T10:53:46Z

Tasks/VsTest/helpers.ts

+    public static getXmlContents(filePath: string): Q.Promise<any> {
+        var defer=Q.defer<any>();
+        Helper.readFileContents(filePath, "utf-8")
+            .then(function (xmlContents) {


why it has to be async? when parseString is simple call back function. 😳

nigurr · 2017-02-17T10:54:28Z

Tasks/VsTest/helpers.ts

+        return fs.readFileSync(filePath, encoding)
+    }
+
+    public static writeXmlFile(result: any, settingsFile: string, fileExt: string): Q.Promise<string> {


sync sync sync

nigurr · 2017-02-17T10:54:56Z

Tasks/VsTest/helpers.ts

+            return defer.promise;
+    }
+
+    public static saveToFile(fileContents: string, extension: string): Q.Promise<string> {


saveToFile generating uuid 👎

nigurr · 2017-02-17T10:55:25Z

Tasks/VsTest/helpers.ts

+    public static getVSVersion(versionNum: number)
+    {
+        switch (versionNum) {
+            case 12: return "2013";


return type?

nigurr · 2017-02-17T10:55:55Z

Tasks/VsTest/models.ts

+    testDropLocation: string; // search folder
+    overrideTestrunParameters: string;
+    codeCoverageEnabled: boolean;
+    videoCoverageEnabled: boolean;


is it being used?

nigurr · 2017-02-17T10:56:09Z

Tasks/VsTest/models.ts

+export interface DtaTestConfigurations extends TestConfigurations {
+    onDemandTestRunId: string;
+    testConfigurationMapping: string; // TODO : What is this?
+    customSlicingenabled: boolean;


nigurr · 2017-02-17T10:56:35Z

Tasks/VsTest/package.json

@@ -18,6 +18,7 @@
  "dependencies": {
    "performance-now": "0.2.0",
    "vsts-task-lib": "^2.0.0-preview",
+	  "vso-node-api": "6.0.4-preview",


nigurr · 2017-02-17T10:56:59Z

Tasks/VsTest/runvstest.ts

@@ -0,0 +1,45 @@
+import tl = require('vsts-task-lib/task');
+import models = require('./models')


Fix tslint issues please

nigurr

Please do tslint fixes and squash all your commits.
Can't review/approve in this state

nigurr · 2017-02-20T05:50:36Z

Please do add a tracking item for Cleanup.

RanjanarMS and others added 30 commits January 25, 2017 15:35

First Level Refactoring of vstest++ task

18b86f7

VSTestPlusPlus with UI

462c853

DTA - VideoCollector and RunInParallel

7c61437

Task Input parser failure

deae236

TIA settings

990984c

Added support for vs2015 for TIA

d327a2b

minor changes

d8955a7

Minor PR comment

04820f2

Refactoring

be4209b

minor fixes

6161a40

minor fixes

33bb2a3

Refactoring

4e4fbf1

In isolation

66dae9e

Logging

c250d9e

Code Review comments

dffa988

test agent fix

ea1559f

test selector change

54818ac

Merge pull request #3539 from Microsoft/users/siddhap/2015vstia

bdcc00f

Users/siddhap/2015vstia

PR comments

d09df08

Merge branch 'users/ranjanar/vstest++_dta' into users/kavipriya/settings

b52a24c

Merge changes

b24dbe2

Merge pull request #3541 from Microsoft/users/kavipriya/settings

7787cf9

Users/kavipriya/settings

Merge pull request #3525 from Microsoft/users/ranjanar/vstest++_dta

544bc5e

VSTestPlusPlus with UI

Test platform version changes for DTA

9f95450

Fixing error messages

31d9427

Minor fix

eca728a

Refactoring

210a710

Changes for max agents

97f76ff

Merge pull request #3562 from Microsoft/users/ranjanar/maxAgentSlicing

644b28c

Changes for max agents

Enabling test plan flow for DTA and VSTest

4fad9cb

Porting ps script execution fix

nigurr reviewed Feb 17, 2017

View reviewed changes

Tasks/VsTest/helpers.ts

}

public static getXmlContents(filePath: string): Q.Promise<any> {

var defer=Q.defer<any>();

Copy link

nigurr Feb 17, 2017

Choose a reason for hiding this comment

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

indention +100

nigurr reviewed Feb 17, 2017

View reviewed changes

Tasks/VsTest/helpers.ts

public static getVSVersion(versionNum: number)

{

switch (versionNum) {

case 12: return "2013";

Copy link

nigurr Feb 17, 2017

Choose a reason for hiding this comment

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

return type?

nigurr reviewed Feb 17, 2017

View reviewed changes

nigurr suggested changes Feb 17, 2017

View reviewed changes

RanjanarMS added 3 commits February 19, 2017 16:48

Merge branch 'master' of https://github.com/Microsoft/vsts-tasks

cf12d99

'merge

279ee73

Fixing L0 and some CR comments

563e92f

nigurr approved these changes Feb 20, 2017

View reviewed changes

RanjanarMS merged commit 7b2160b into master Feb 20, 2017

RanjanarMS deleted the users/ranjanar/formaster branch March 6, 2017 04:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users/ranjanar/formaster #3643

Users/ranjanar/formaster #3643

RanjanarMS commented Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017 •

edited

Loading

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr Feb 17, 2017

nigurr left a comment

nigurr commented Feb 20, 2017

		@@ -0,0 +1,45 @@
		import tl = require('vsts-task-lib/task');
		import models = require('./models')

Users/ranjanar/formaster #3643

Users/ranjanar/formaster #3643

Conversation

RanjanarMS commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nigurr Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nigurr left a comment

Choose a reason for hiding this comment

nigurr commented Feb 20, 2017

nigurr Feb 17, 2017 •

edited

Loading