Skip to content

Conversation

@vladimir-shcherbakov
Copy link
Contributor

@vladimir-shcherbakov vladimir-shcherbakov commented Jan 10, 2019

Description

Checklist

MiYanni
MiYanni previously approved these changes Jan 23, 2019
Copy link
Contributor

@MiYanni MiYanni left a comment

Choose a reason for hiding this comment

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

LGTM

@MiYanni
Copy link
Contributor

MiYanni commented Jan 23, 2019

@vladimir-shcherbakov There are merge conflicts.

@vladimir-shcherbakov
Copy link
Contributor Author

@vrdmr We are applying a new test controller - could you take a look?

@vrdmr
Copy link
Member

vrdmr commented Jan 28, 2019

@sourabhguha, @GaneshMSAzure, @NarayanThiru, @kjohn-msft, @safeermohammed, @AnatoliB, @jemex, @NicoletazMS and @finiteattractor: FYI - New Test Controller for Automation cmdlets

Copy link
Member

@vrdmr vrdmr left a comment

Choose a reason for hiding this comment

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

LGTM

@vladimir-shcherbakov
Copy link
Contributor Author

@vladimir-shcherbakov
Copy link
Contributor Author

@MiYanni Are you ready to sign off?

@MiYanni
Copy link
Contributor

MiYanni commented Jan 31, 2019

@vladimir-shcherbakov Please reassign to someone else. Thanks.

@MiYanni MiYanni removed their assignment Jan 31, 2019
Copy link
Member

@markcowl markcowl left a comment

Choose a reason for hiding this comment

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

Minor comments about the namespace name for the new class, aso, it would be nice if all the usings weher outside namespace declarations. otherwise LGTM

// ----------------------------------------------------------------------------------

using Microsoft.Azure.Commands.Network.Test.ScenarioTests;

Copy link
Member

Choose a reason for hiding this comment

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

Please put all the usings either inside or outside the namespace

are recorded. See example in SourceControlTests.ps1 where $testReposInfo is defined.
*/

using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same comment

// limitations under the License.
// ----------------------------------------------------------------------------------

using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same comment

// limitations under the License.
// ----------------------------------------------------------------------------------

using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same comment

// limitations under the License.
// ----------------------------------------------------------------------------------

using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same


using Microsoft.Azure.Commands.Automation.Test;
using Microsoft.Azure.ServiceManagement.Common.Models;
using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

Why is the using for the network scenario tests required?

using Microsoft.WindowsAzure.Commands.ScenarioTest;
using Xunit.Abstractions;

namespace Microsoft.Azure.Commands.Network.Test.ScenarioTests
Copy link
Member

Choose a reason for hiding this comment

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

The namespace here should be Microsoft.Azure.Commands.Automation.Test or Commands.Automation.Test

// ----------------------------------------------------------------------------------

using Microsoft.Azure.ServiceManagement.Common.Models;
using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

see namepspace comment above

using System.Threading.Tasks;
using Microsoft.Azure.Commands.Automation.Test;
using Microsoft.Azure.ServiceManagement.Common.Models;
using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same comment

using System.Threading.Tasks;
using Microsoft.Azure.Commands.Automation.Test;
using Microsoft.Azure.ServiceManagement.Common.Models;
using Microsoft.Azure.Commands.Network.Test.ScenarioTests;
Copy link
Member

Choose a reason for hiding this comment

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

same comment, and for all those below

@markcowl markcowl merged commit b95b60b into Azure:master Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.