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

Convert LocalTests_SettingsModel into UnitTests_SettingsModel #7743

Open
carlos-zamora opened this issue Sep 25, 2020 · 0 comments
Open

Convert LocalTests_SettingsModel into UnitTests_SettingsModel #7743

carlos-zamora opened this issue Sep 25, 2020 · 0 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Milestone
Engineering Impro...

Comments

@carlos-zamora
Copy link
Member

Description of the new feature/enhancement

#7667 introduces the TerminalSettingsModel by ripping out a large chunk of TerminalApp. All of the testing was thrown into LocalTests_SettingsModel. Moving them into a unit tests project like TermApp.UnitTests allows us to run these in CI.

About 20/33 tests are able to be moved into a UnitTests-like project with no modifications.
2/33 can be moved with minor modifications (see below).
The remaining 11/33 require a larger refactor (this issue tracks this).

Proposed technical implementation details (optional)

SettingsModelLocalTests::KeyBindingsTests

  • All of these tests fail because they need to construct a KeyChord. KeyChord is in the TerminalControl namespace.

SettingsModelLocalTests::CommandTests

  • TestResourceKeyName and TestLayerOnAutogeneratedName fail because they need access to the resource loader.

SettingsModelLocalTests::SerializationTests

  • The tests below fail because they need to import defaults.json. Which results in hitting the two cases above.
    • TestReorderWithNullGuids
    • TestReorderingWithoutGuid
    • TestLayeringNameOnlyProfiles
    • TestExplodingNameOnlyProfiles
    • TestDontLayerGuidFromUserDefaults
    • ValidateKeybindingsWarnings
  • TestCommandsAndKeybindings doesn't import defaults.json, but fails again because it hits the two cases above.
  • The two tests that can be fixed fairly easily are ValidateLegacyGlobalsWarning and TestTrailingCommas. They don't really need a dependency on importing defaults.json.
@carlos-zamora carlos-zamora added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Sep 25, 2020
@carlos-zamora carlos-zamora added this to the Terminal Backlog milestone Sep 25, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 25, 2020
@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 Sep 25, 2020
@carlos-zamora carlos-zamora mentioned this issue Sep 25, 2020
Merged
carlos-zamora added a commit that referenced this issue Oct 6, 2020
@carlos-zamora
Introduce TerminalSettingsModel project (#7667)
2608e94 
Introduces a new TerminalSettingsModel (TSM) project. This project is
responsible for (de)serializing and exposing Windows Terminal's settings
as WinRT objects.

## References
#885: TSM epic
#1564: Settings UI is dependent on this for data binding and settings access
#6904: TSM Spec

In the process of ripping out TSM from TerminalApp, a few other changes
were made to make this possible:
1. AppLogic's `ApplicationDisplayName` and `ApplicationVersion` was
   moved to `CascadiaSettings`
   - These are defined as static functions. They also no longer check if
     `AppLogic::Current()` is nullptr.
2. `enum LaunchMode` was moved from TerminalApp to TSM
3. `AzureConnectionType` and `TelnetConnectionType` were moved from the
   profile generators to their respective TerminalConnections
4. CascadiaSettings' `SettingsPath` and `DefaultSettingsPath` are
   exposed as `hstring` instead of `std::filesystem::path`
5. `Command::ExpandCommands()` was exposed via the IDL
   - This required some of the warnings to be saved to an `IVector`
     instead of `std::vector`, among some other small changes.
6. The localization resources had to be split into two halves.
   - Resource file linked in init.cpp. Verified at runtime thanks to the
     StaticResourceLoader.
7. Added constructors to some `ActionArgs`
8. Utils.h/cpp were moved to `cascadia/inc`. `JsonKey()` was moved to
   `JsonUtils`. Both TermApp and TSM need access to Utils.h/cpp.

A large amount of work includes moving to the new namespace
(`TerminalApp` --> `Microsoft::Terminal::Settings::Model`).

Fixing the tests had its own complications. Testing required us to split
up TSM into a DLL and LIB, similar to TermApp. Discussion on creating a
non-local test variant can be found in #7743.

Closes #885
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-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Engineering Improvements
Development

No branches or pull requests
2 participants
@carlos-zamora @zadjii-msft