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

Windows setting language #120

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Nov 7, 2024


Addresses issue #34 and #92. Only the module this time.

Microsoft Reviewers: Open in CodeFlow

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 7, 2024

@stephengillie Opened up this one. Hope you don't mind the docs for the module here.

@Gijsreyn Gijsreyn force-pushed the windows-setting-language branch 2 times, most recently from fdd3054 to b482a12 Compare November 14, 2024 10:45
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback This needs a response from the author. and removed Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Nov 15, 2024
@Gijsreyn Gijsreyn force-pushed the windows-setting-language branch from 9d56ef3 to 41a4038 Compare December 2, 2024 00:13
@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 2, 2024

@ryfu-msft You can put this on hold. I'm seeing some strange behaviours with the relevant modules used to set language, plus a bug found on Windows PowerShell: PowerShell/PowerShellModuleCoverage#18.

@denelon I might need your help, but I'll test thoroughly first and let you know.

@denelon denelon marked this pull request as draft December 2, 2024 15:49
@denelon
Copy link
Contributor

denelon commented Dec 2, 2024

@Gijsreyn I converted this to a draft PR until you're ready for review/merge.

@stephengillie
Copy link
Collaborator

Apologies for my absent repsonse - I've been too busy maintaining the winget-pkgs repo, to be able to review this and provide feedback with constructive value.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 7, 2024

Apologies for my absent repsonse - I've been too busy maintaining the winget-pkgs repo, to be able to review this and provide feedback with constructive value.

Thanks for letting me know and no worries. We can't do everything at the same time.

This comment has been minimized.

@Gijsreyn Gijsreyn marked this pull request as ready for review December 8, 2024 18:12
@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 8, 2024

@ryfu-msft I know you're also quite busy, but if you have time to recheck this one, I would appreciate it. I incorporated as much feedback as you gave me earlier. If I've missed something, then I apologize. I have rewritten many parts and tested the logic on a fresh box. Many thanks for considering my request.

P.S. I'm putting other settings regarding the region on hold. That way, the PR doesn't get that big

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@ryfu-msft ryfu-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks good to me! I think the tests are showing that you have some module dependency issues as well as a couple lines to fix in the tests.

@microsoft-github-policy-service microsoft-github-policy-service bot added Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Dec 11, 2024
@Gijsreyn
Copy link
Contributor Author

The implementation looks good to me! I think the tests are showing that you have some module dependency issues as well as a couple lines to fix in the tests.

Fixed up the tests. I think I was a bit to fast. Added the import module statement now. Can you give it another go?

@microsoft-github-policy-service microsoft-github-policy-service bot removed Changes-Requested Changes Requested Needs-Author-Feedback This needs a response from the author. labels Dec 11, 2024

This comment has been minimized.

@Gijsreyn Gijsreyn requested a review from ryfu-msft December 11, 2024 09:02
@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 11, 2024

@ryfu-msft This is a bit out of my league. Clearly, the image that is being used does not have the LanguagePackmanagement built-in. I cannot find anywhere to install it. My best bet would be to either add it directly to the repository, which is quite dirty, or mock the Pester tests. Any other suggestions are welcome.

EDIT 2: After asking the question on azure-pipelines-agent repository, I found out this module is only for clients.

@ryfu-msft
Copy link
Contributor

@ryfu-msft This is a bit out of my league. Clearly, the image that is being used does not have the LanguagePackmanagement built-in. I cannot find anywhere to install it. My best bet would be to either add it directly to the repository, which is quite dirty, or mock the Pester tests. Any other suggestions are welcome.

EDIT 2: After asking the question on azure-pipelines-agent repository, I found out this module is only for clients.

Ah I see, if that is the case then you can try mocking the result of the function that uses the LanguagePackManagement so that it doesn't get invoked. The other option is the remove parts of the tests that require LanguagePackageManagement. We will attempt to have as much coverage as we can since this this is not supported on server builds.

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Dec 21, 2024

@ryfu-msft This is a bit out of my league. Clearly, the image that is being used does not have the LanguagePackmanagement built-in. I cannot find anywhere to install it. My best bet would be to either add it directly to the repository, which is quite dirty, or mock the Pester tests. Any other suggestions are welcome.
EDIT 2: After asking the question on azure-pipelines-agent repository, I found out this module is only for clients.

Ah I see, if that is the case then you can try mocking the result of the function that uses the LanguagePackManagement so that it doesn't get invoked. The other option is the remove parts of the tests that require LanguagePackageManagement. We will attempt to have as much coverage as we can since this this is not supported on server builds.

Okay, after trying multiple approaches, I will give this one up. The reasoning being is the following:

  • When mocking, it tells me that the cmdlet is not available
  • Other errors that are thrown are related to a bug; see image below

image

The only thing that remains is a different OS, which changes the Azure Pipelines. I have commented out all tests, expect the validation of DSC resources. Sorry Ryan. If you have any more suggestions, they are welcome, but I don't see anyway currently to get it working on the hosted agents.

P.S. The code I tried (thanks to Trenly):

InModuleScope -ModuleName Microsoft.Windows.Setting.Language {
    Describe 'Language' {
        BeforeAll {
            Mock Get-Language { return @{Language = 'en-GB'; } }
            Mock Install-Language { return $null }
        }
 
        $script:LanguageResource = [Language]::new()
 
        Context 'Install Language' -Tag 'Set' {
            It 'Should install a language' {
                $LanguageResource.LanguageId = 'en-GB'
                { $LanguageResource.Set() } | Should -Not -Throw
            }
        }

        Context 'Uninstall Language' -Tag 'Set' {
            It 'Should uninstall a language' {
                $LanguageResource.LanguageId = 'en-GB'
                $LanguageResource.Exist = $false
                { $LanguageResource.Set() } | Should -Not -Throw
            }
        }
 
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants