-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Track1 testfx re-design and improvement #18720
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
Conversation
documentation/development-docs/azure-powershell-developer-guide.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall it be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AADTenantKey is ambiguous while TenantIdKey is more straightforward. It only involved a few changes. Recommend updating the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend that your don't need to rename it even it doesn't follow convention. It can reduce the size of your pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the naming convention for the TestRunner file is {Test-Project-Name} + TestRunner. We have almost all the test projects that comply with this standard except for this one. In order to make all the test runner names consistent within the whole repo, I strongly believe this change is necessary. I concede it would reduce the size of this PR. But since it is necessary, better we can keep the class name as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, it is not required. Please check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add method RunPowerShellTest in super class SqlTestRunner. Then you don't need to change below each line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the TestRunner.RunTestScript with the RunPowerShellTest method is not that good because it is just a wrapper without any other function like concealing the real API. If some of the test cases need to pass in additional parameters later on, the wrapper won't work. In addition, adding a wrapper cannot reduce the size of the PR as all these classes must inherit from SqlTestRunner. This is a required change.
This PR has tremendous number of changes regarding the Test Framework for all the track1 management plane cmdlets. All new test cases will opt for TestRunner as the unified approach.
Description
Work done in this PR.
ConnectionStringKeys.csfile, renamed fromAADTenantKeytoTenantIdKeybecause the original name is ambiguous and the new one is more straightfrorward. Also updated the places where this key was referenced.Microsoft.Azure.Test.HttpRecorderandMicrosoft.Rest.ClientRuntime.Azure.TestFrameworkin Azure SDK fromCommon.Netcore.Dependencies.Test.targetsTestProjectPathinAz.Test.propsfalseinAz.Shared.propsResources.Testproject, renamed the TestRunner fromResourceTestRunnertoResourcesTestRunnerto unify the naming convention of the TestRunner, which is{{TestProjectName}}TestRunner. Also updated the subclasses where the this parent was inherited.Checklist
CONTRIBUTING.mdChangeLog.mdfile(s) has been updated:ChangeLog.mdfile can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md## Upcoming Releaseheader -- no new version header should be added