-
Notifications
You must be signed in to change notification settings - Fork 177
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
Move get-codeowners.ps1
test mode to common-tests
+ related improvements & cleanup
#5608
Conversation
1ed0a49
to
3d46d04
Compare
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
eng/common/pipelines/templates/stages/archetype-sdk-tool-pwsh.yml
Outdated
Show resolved
Hide resolved
The following pipelines have been queued for testing: |
6b93176
to
01c5f86
Compare
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
eng/common-tests/README.md
Outdated
|
||
## When tests in this directory are executed | ||
|
||
The PowerShell Pester unit tests will be executed by the [`tools - eng-common-tests`](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=220791) pipeline |
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.
Does this need to be an internal pipeline? If it doesn't require secrets which I don't believe it does we should move it to public. That way we can have it automatically trigger for any PR.
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 created a public equivalent:
https://dev.azure.com/azure-sdk/public/_build?definitionId=5985&_a=summary
Once this PR is merged, I will delete the internal version:
https://dev.azure.com/azure-sdk/internal/_build?definitionId=5809
(update: deleted as of 3/30/2023, but since about at least 2 weeks before)
and also the internal tools eng-script-tests
, which appears redundant to me:
https://dev.azure.com/azure-sdk/internal/_apps/hub/ms.vss-build-web.ci-designer-hub?pipelineId=5823
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.
Yes we can combine them if they are doing similar things.
For the public pipeline we generally add the "- ci" postfix. So tools - eng-common-tests - ci
or tools - script-tests - ci
if we make it more generic.
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.
Renamed to tools - eng-common-tests - ci
. The tools - script-tests
looks to be totally redundant to me, just a subset of eng-common-tests
, so I will outright delete it.
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.
OK correction: the script-tests
was running tests in eng/scripts
but we have since moved all such tests to eng/common-tests
. I confirmed this by invoking pester on eng/scripts
- it found no tests to run.
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.
OK I'm supportive of having just the one script testing pipeline.
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.
@weshaggard ungh, actually, there are tests, in:
eng\scripts\contentreplacements\tests\Repo-File-Content-Replacements\Repo-File-Content-Replacements.Tests.ps1
I was just running pester against scripts
instead of eng/scripts
. I have a task to merge these dirs.
In any case, I cannot delete that pipeline yet. But I am thinking to take all the tests in eng/scripts
(which is just the one file above) and move them to eng/common-tests
and rename eng/common-tests
to e.g. eng/powershell-tests
.
Does that sound reasonable?
If so, I'll make a separate PR for that, and for now hold off with any changes to the tools - script-tests
pipeline.
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.
Two things.
1. PowerShell test pipeline setup
I had a chat with @weshaggard. We should keep multiple pipelines
- one pipeline for running PowerShell tests in
/eng/common-tests/
; - one pipeline per each language repo for running PowerShell tests in its
/eng/scripts/
dir.
This is because:
- There are two separate sets of PowerShell scripts:
/eng/common/scripts/
and/eng/scripts/
and both these sets need to have tests written for them. - The dir
/eng/scripts/
in each language repo has language-repo-specific scripts; currently they hardly have any tests, but in the future they should have more tests.
The pipelines can be sourced from different ci.yml
files - the ci.yml
source would differ only by parameters passed to the underlying abstraction for running Pester tests. Most importantly, the TargetDirectory
would point to appropriate dir.
2. PowerShell file naming conventions
Per chat in this PR and over Teams between me, @weshaggard and @benbp we want to adopt following convention for naming files:
/eng/scripts/foo.ps1
and/eng/common/scripts/foo.ps1
for directly-invocable entry points;/eng/scripts/foo.lib.ps1
and/eng/common/scripts/foo.lib.ps1
for dot-sourceable functions; thefoo.ps1
is expected to be in practice just a delegate calling the "entry point" function fromfoo.lib.ps1
;/eng/scripts/foo.tests.ps1
and/eng/common-tests/foo.tests.ps1
for Pester tests forfoo.lib.ps1
.
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.
Created an issue to track the work above:
The following pipelines have been queued for testing: |
get-codeowners
test mode to common-tests
+ related improvements & cleanup
get-codeowners
test mode to common-tests
+ related improvements & cleanupget-codeowners.ps1
test mode to common-tests
+ related improvements & cleanup
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
0b0867d
to
576805b
Compare
The following pipelines have been queued for testing: |
…ncludeNonUserAliases as switch
6bf6d2c
to
ffd9eed
Compare
The following pipelines have been queued for testing: |
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5608 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Konrad Jamrozik <kojamroz@microsoft.com>
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5608 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Konrad Jamrozik <kojamroz@microsoft.com>
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5608 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Konrad Jamrozik <kojamroz@microsoft.com>
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5608 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Konrad Jamrozik <kojamroz@microsoft.com>
update the version and changelog update api update api view fix tests [engsys] Upgrade dev dependency `dotenv` version to `^16.0.0` for selected packages (#24020) Update eng/common/scripts/get-codeowners.ps1 Update get-codeowners.ps1 update Sync eng/common directory with azure-sdk-tools for PR 5608 (#25133) Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#5608 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: Konrad Jamrozik <kojamroz@microsoft.com> update changelog Update eng/common/scripts/get-codeowners.ps1 update update
This PR moves the
Test
flag away fromget-codeowners.ps1
to the establishedeng/common-tests
infrastructure, using PowerShell Pester testing framework. As such, it solves:test
flag fromget-codeowners.ps1
to the common PS1 test infra,common-tests
#5434This PR also:
eng/common-tests
;get-codeowners.lib.ps1
, so that the functions can be reused between normalget-codewners.ps1
usage and through Pester unit test invocations coming fromget-codeowners.tests.ps1
;get-codeowners.lib.ps1
and hides some unused params; this was necessary to avoid default param value duplication between calls fromget-codeowners.ps1
and fromget-codeowners.tests.ps1
;Associated changes made manually once this PR got merged:
Follow-up work:
/eng/scripts/
+ fix naming convention #5878