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

Remove VerifyRSxxDictionariesWereMergedCorrectly test cases #3935

Merged
merged 5 commits into from
Jan 14, 2021

Conversation

licanhua
Copy link
Contributor

The merge dictionary test cases like VerifyRS2ThemeResourceDictionariesWereMergedCorrectly become more and more difficult to maintain because:

  1. More and more styles are consolidated to a single file by the unofficial conditional Xaml which Xaml compiler doesn’t support
  2. Resource may be shared and across multiple files. For example, the brushes may be created in CommonStyles, then used by all controls.

In the other hand, the mergedictionary functionality is very stable and is integrated to CustomTask and we have less chance to modify it.

So this PR:

  1. remove those test cases
  2. remove UseNonstandardCondtionalXAML which was working as an workaround to make test cases pass
  3. add comment to BatchMergeXaml.cs

Benefit:

  1. Easy to maintain
  2. small performance improvement to load and build the test project

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jan 12, 2021
@licanhua
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-TestInfrastructure Issue in the test infrastructure (e.g. in Helix scripts) team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Jan 13, 2021
@ranjeshj ranjeshj requested a review from kmahone January 13, 2021 16:37
@kmahone
Copy link
Member

kmahone commented Jan 13, 2021

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

@llongley
Copy link
Member

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

That's my recollection as well. I don't remember the exact point where conditional XAML became supported, but I believe it was RS3 or RS4. So we basically needed to handle conditional XAML ourselves rather than letting the XAML parser handle it.

@licanhua
Copy link
Contributor Author

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

That's my recollection as well. I don't remember the exact point where conditional XAML became supported, but I believe it was RS3 or RS4. So we basically needed to handle conditional XAML ourselves rather than letting the XAML parser handle it.

To be exactly, Conditional Xaml has some restrictions:

  1. RS2 and RS3 has some bugs
  2. It can't be applied everywhere. For example:
<contract7NotPresent:StaticResource x:Key="AutoSuggestBoxSuggestionsListBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />

It was a nightmare to support density, lift controls from os.
Conditional Xaml was the solution to go, for example, CommandBar was the 1st control to use it.
In practice, I found bugs to apply conditional Xaml on RS2 and RS3, also it can't apply conditional everywhere.
So I finally introduced the nonofficial conditional xaml: Scripts are used to strip the namespace before Xaml is compiled.

@kmahone
Copy link
Member

kmahone commented Jan 14, 2021

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

That's my recollection as well. I don't remember the exact point where conditional XAML became supported, but I believe it was RS3 or RS4. So we basically needed to handle conditional XAML ourselves rather than letting the XAML parser handle it.

To be exactly, Conditional Xaml has some restrictions:

  1. RS2 and RS3 has some bugs
  2. It can't be applied everywhere. For example:
<contract7NotPresent:StaticResource x:Key="AutoSuggestBoxSuggestionsListBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />

It was a nightmare to support density, lift controls from os.
Conditional Xaml was the solution to go, for example, CommandBar was the 1st control to use it.
In practice, I found bugs to apply conditional Xaml on RS2 and RS3, also it can't apply conditional everywhere.
So I finally introduced the nonofficial conditional xaml: Scripts are used to strip the namespace before Xaml is compiled.

So, it sounds like we still need non-standard conditional xaml then? Is your change removing it from the files? I see lots of removals of " ". It might be that I am misinterpreting the change.

@licanhua
Copy link
Contributor Author

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

That's my recollection as well. I don't remember the exact point where conditional XAML became supported, but I believe it was RS3 or RS4. So we basically needed to handle conditional XAML ourselves rather than letting the XAML parser handle it.

To be exactly, Conditional Xaml has some restrictions:

  1. RS2 and RS3 has some bugs
  2. It can't be applied everywhere. For example:
<contract7NotPresent:StaticResource x:Key="AutoSuggestBoxSuggestionsListBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />

It was a nightmare to support density, lift controls from os.
Conditional Xaml was the solution to go, for example, CommandBar was the 1st control to use it.
In practice, I found bugs to apply conditional Xaml on RS2 and RS3, also it can't apply conditional everywhere.
So I finally introduced the nonofficial conditional xaml: Scripts are used to strip the namespace before Xaml is compiled.

So, it sounds like we still need non-standard conditional xaml then? Is your change removing it from the files? I see lots of removals of " ". It might be that I am misinterpreting the change.

Are you talking about UseNonstandardCondtionalXAML? Only the concerned test cases are using this tag.
We still use Nonstandard conditional Xaml in merge tool in CustomTask, but it doesn't care about this tag. The mergetool treat all pages as nonstandard conditional Xaml pages.

@kmahone
Copy link
Member

kmahone commented Jan 14, 2021

My understanding is that we needed NonStandardConditionalXaml because conditional xaml was not fully operational down-level (e.g. RS2). @llongley can you remember the details here?

That's my recollection as well. I don't remember the exact point where conditional XAML became supported, but I believe it was RS3 or RS4. So we basically needed to handle conditional XAML ourselves rather than letting the XAML parser handle it.

To be exactly, Conditional Xaml has some restrictions:

  1. RS2 and RS3 has some bugs
  2. It can't be applied everywhere. For example:
<contract7NotPresent:StaticResource x:Key="AutoSuggestBoxSuggestionsListBackground" ResourceKey="SystemControlBackgroundChromeMediumLowBrush" />

It was a nightmare to support density, lift controls from os.
Conditional Xaml was the solution to go, for example, CommandBar was the 1st control to use it.
In practice, I found bugs to apply conditional Xaml on RS2 and RS3, also it can't apply conditional everywhere.
So I finally introduced the nonofficial conditional xaml: Scripts are used to strip the namespace before Xaml is compiled.

So, it sounds like we still need non-standard conditional xaml then? Is your change removing it from the files? I see lots of removals of " ". It might be that I am misinterpreting the change.

Are you talking about UseNonstandardCondtionalXAML? Only the concerned test cases are using this tag.
We still use Nonstandard conditional Xaml in merge tool in CustomTask, but it doesn't care about this tag. The mergetool treat all pages as nonstandard conditional Xaml pages.

That makes sense. Thanks for the clarification.

@licanhua licanhua merged commit 3290f0c into master Jan 14, 2021
@licanhua licanhua deleted the user/canli/removetest branch January 14, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TestInfrastructure Issue in the test infrastructure (e.g. in Helix scripts) team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants