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

[Test Controller] New test controller applied to Network #8281

Merged
merged 5 commits into from
Feb 13, 2019

Conversation

vladimir-shcherbakov
Copy link
Contributor

Description

Checklist

@@ -24,7 +24,6 @@

<ItemGroup>
<None Update="ScenarioTests\Data\*.*" CopyToOutputDirectory="PreserveNewest" />
<None Update="ScenarioTests\Generated\*.*" CopyToOutputDirectory="PreserveNewest" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were the generated scenario tests moved up a directory? They put them there so that only generated tests are in that folder, and they manage non-generated tests in the ScenarioTests folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agreed these changes with the Network team

@@ -1,6 +0,0 @@
ApplicationSecurityGroupTestsGenerated.cs 299A1E5866F1F33032F70699B1B9A1790689CD35
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file deleted? They use a generator to create some of their tests. I'm assuming this file is used by their generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agreed these changes with the Network team

Copy link
Member

Choose a reason for hiding this comment

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

This captures the commit for the source script they used to generate the tests, so we should leave it, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@EvgenyAgafonchikov Can you confirm you need the metadata?

Copy link
Contributor Author

@vladimir-shcherbakov vladimir-shcherbakov Feb 11, 2019

Choose a reason for hiding this comment

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

I discussed this offline with @EvgenyAgafonchikov:

AFAIK PS team plans to use their own cmdlets generator, so we are dropping support of ours. In this context removing of this file is okay.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't apply until we are actually using a new generator.

@vladimir-shcherbakov
Copy link
Contributor Author

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

Copy link
Contributor

@EvgenyAgafonchikov EvgenyAgafonchikov left a comment

Choose a reason for hiding this comment

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

Left comment on module reference. Otherwise LGTM.

@vladimir-shcherbakov
Copy link
Contributor Author

@MiYanni Are you ready to sign off?

@vladimir-shcherbakov
Copy link
Contributor Author

@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
}

[Fact]
[Trait(Category.RunType, Category.LiveOnly)]
[Trait(Category.Owner, Category.netanalyticsdev)]
public void TestFlowLog()
{
NetworkResourcesController.NewInstance.RunPsTest(_logger, "Test-FlowLog");
TestRunner.RunTestScript("Test-FlowLog");
}

#if NETSTANDARD
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this #if - we are awlays in NetStandard

@@ -1,6 +0,0 @@
ApplicationSecurityGroupTestsGenerated.cs 299A1E5866F1F33032F70699B1B9A1790689CD35
Copy link
Member

Choose a reason for hiding this comment

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

This captures the commit for the source script they used to generate the tests, so we should leave it, I think

@vladimir-shcherbakov vladimir-shcherbakov removed their assignment Feb 11, 2019
@@ -1,6 +0,0 @@
ApplicationSecurityGroupTestsGenerated.cs 299A1E5866F1F33032F70699B1B9A1790689CD35
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't apply until we are actually using a new generator.

@markcowl markcowl dismissed MiYanni’s stale review February 13, 2019 23:11

handled by network team

@markcowl markcowl merged commit 0938096 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.

7 participants