-
Notifications
You must be signed in to change notification settings - Fork 254
Fix Missing Test Apps after Import-TestToolkitToBcContainer #3841
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
base: main
Are you sure you want to change the base?
Conversation
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 am not sure that adding these:
'Microsoft_Library-NoTransactions.app',
'Microsoft_Prevent Metadata Updates.app'
won't break all tests.
I think the no transactions is an app you an install to make sure that no transactions are made while running tests - and the same with metadata.
@ChrisBlankDe - have you tested this? As already mentioned - I assume that all tests will stop working if I install the NoTransactions app and if there is one thing I really don't want to do, then it is to merge a PR, which breaks the test run of every single partner on planet earth:-) |
Just double checked this. |
First of all, you are right that it is a mess:-)
But, if any test changes system tables they will fail with prevent metadata updates on. On the applications - have you tested that these test apps are running? I excluded a number of test apps, which shouldn't be run. Lastly - if the SMTP Test API library should be included, it also needs to be included in the compilerfolder code. |
I converted this PR back to draft. i will try to implement this also for compilation folder (just used by run-alpipeline :/ ) |
…est libraries and test apps
…simplify filtering logic
i have revised the PR again and tested it properly. the description has been supplemented so that it is now obvious which apps have changed. |
@mazhelez would it be possible for you to review this? |
Will do. Thanks for tagging me, as I haven't developed a habit to monitor PRs in this repo yet. |
@@ -457,9 +458,19 @@ function GetTestToolkitApps { | |||
@(get-childitem -Path "C:\Applications\*.*" -recurse -filter $_) | |||
} | |||
} | |||
$apps += @(get-childitem -Path "C:\Applications\*.*" -recurse -filter "Microsoft_Tests-*.app") | Where-Object { $_ -notlike "*\Microsoft_Tests-TestLibraries.app" -and ($version.Major -ge 17 -or ($_ -notlike "*\Microsoft_Tests-Marketing.app")) -and $_ -notlike "*\Microsoft_Tests-SINGLESERVER.app" } | |||
$apps += @(get-childitem -Path "C:\Applications\*.*" -recurse) | ? { $_.name -like "Microsoft_Tests-*.app" -or $_.FullName -match '\\Test\\.*[ _]Test(?:s?| Automations).app' } | Where-Object { ($version.Major -ge 17 -or ($_ -notlike "*\Microsoft_Tests-Marketing.app")) } |
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.
It almost looks like it's easier just to explicitly list what to include and/or exclude.
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 was not sure about this while implementing. i decide for this solution so we are not forced to change this when msft adds a new app. on the other side, it is not possible to see at a quick glance which apps are published
as pr description says this would have been simpler / nicer if msft had a consistent folder structure and naming pattern :/
we currently have around 82 test apps and 12 libraries
if you prefer to list all apps just say a word.
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.
as pr description says this would have been simpler / nicer if msft had a consistent folder structure and naming pattern :/
That we definitely agree on. But this is also not something we can change overnight.
Both explicitly listing the apps to include/exclude and matching the name with a patter have advantages and disadvantages.
I am more leaning towards the former as:
- it's easier to get an overview which apps are included/excluded
- new apps are not being added by default, which might be a good thing as there'll be no side effects.
What do you think?
Co-authored-by: Maria Zhelezova <43066499+mazhelez@users.noreply.github.com>
Fix #3729
this would have been simpler / nicer if msft had a consistent folder structure and naming pattern :/
Symbols: