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.

Sign up for GitHub

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

Jump to bottom

SettingsModelLocalTests failures are on the rise #14050

Closed
j4james opened this issue Sep 20, 2022 · 3 comments · Fixed by #14064
Closed

SettingsModelLocalTests failures are on the rise #14050

j4james opened this issue Sep 20, 2022 · 3 comments · Fixed by #14064
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.17

Comments

@j4james
Copy link
Collaborator

j4james commented Sep 20, 2022

Windows Terminal version

Commit 1ce22a8

Windows build number

10.0.19044.1889

Other Software

No response

Steps to reproduce

!. Check out a recent version of the project.
2. Build it.
3. Run the unit tests.

Expected Behavior

All tests should pass.

Actual Behavior

A number of SettingsModelLocalTests are failing:

SettingsModelLocalTests::DeserializationTests::TestCloseOnExitParsing [Failed]
SettingsModelLocalTests::SerializationTests::Profile [Failed]
SettingsModelLocalTests::SerializationTests::Actions [Failed]
SettingsModelLocalTests::SerializationTests::LegacyFontSettings [Failed]

If I remember correctly, the first one has been around for a while now. I'm afraid I've just been ignoring it and hoping somebody else would notice. The others I think are fairly recent. Although I may be misremembering which are old and which are new.
@j4james j4james added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 20, 2022
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 20, 2022
@j4james
Copy link
Collaborator Author

j4james commented Sep 20, 2022

Looking at the test logs for those last three, it seems the failures are a result of a change in serialization of the font size from int to float (I suspect PR #14013).

The older one I suspect is the result of the new Automatic mode for CloseOnExit (PR #13560).

@lhecker
Copy link
Member

lhecker commented Sep 21, 2022

I keep forgetting that we don't run them in the CI... 😞

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Sep 23, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.17 milestone Sep 23, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 23, 2022
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 18, 2022
@zadjii-msft zadjii-msft mentioned this issue Nov 16, 2022
Merged
7 tasks
@ghost ghost added the In-PR This issue has a related PR label Nov 16, 2022
@ghost ghost closed this as completed in #14064 Dec 6, 2022
ghost pushed a commit that referenced this issue Dec 6, 2022
@bennettnicholas
Add support for switching the scheme based on the app's theme (#14064)
da2b80b 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
This pull request solved the problem of users not being able to set color schemes specifically for dark or light mode. Now the code has been updated to accept a dark and light color scheme in the json. The old setting is still compatible. Keep in mind if you update your color scheme through the settings UI, it will set both dark and light to the color scheme selected. This is because the settings UI update for selecting both Dark and Light color schemes is not supported yet.

This also solves the problem of the UI not using the system OS theme. Now you can select system theme and your color scheme will be selected based on if the system theme is dark or light.


<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References
#4066 

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4066 
* [x] Closes #14050
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA. 
* [x] Tests added/passed I believe so, added one test to ColorSchemeTests.cpp and I believe it passed. Also had to modify TerminalSettingsTests.cpp to accept the new ApplyAppearanceSettings function template
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] Schema updated.
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #4066 and also teams messages with @carlos-zamora 

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
-Removed ColorSchemeName from MTSMSettings.h in order to process the setting for both string and object.
-Added DarkColorSchemeName and LightColorSchemeName properties to the AppearanceConfig to replace ColorSchemeName.
-Hacked a few processes to play nice with all 3 properties listed above as in some cases around the UI, we need to still use the ColorSchemeName. Once we change the UI I believe we can go back to just Dark and LightColorSchemeName
-Added and Updated Test to align to the new code.

Acceptable Json values,

"colorScheme": 
                {
                    "dark": "Campbell",
                    "light": "Campbell"
                }
or

"colorScheme": "Campbell"

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Individual testing along with the test case added.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 6, 2022
@ghost
Copy link

ghost commented Jan 24, 2023

🎉This issue was addressed in #14064, which has now been successfully released as Windows Terminal Preview v1.17.1023.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.17
Development

Successfully merging a pull request may close this issue.

Add support for switching the scheme based on the app's theme bennettnicholas/terminal
4 participants
@lhecker @j4james @carlos-zamora @zadjii-msft