DmitriiBobreshev
commented
1 year ago Hi @echalone
Thank you again for noticing that and thanks a lot for the PR, we will test & review it when we will have a little bit more time.
Open echalone opened 1 year ago
DmitriiBobreshev
commented
1 year ago Hi @echalone
Thank you again for noticing that and thanks a lot for the PR, we will test & review it when we will have a little bit more time.
ismayilov-ismayil
commented
1 year ago Hi @echalone. Many thanks for your reporting and PR. I reviewed and tested your code, it is OK. I mentioned it in PR itself but somehow it is missed. Your PR is for two issues same time. Could you please separate it to two PRs? It will be easy to merge one by one. Thanks
echalone
commented
1 year ago Hello @ismayilov-ismayil Thank you for your time! I will happily do so as soon as I am able to again. I'm currently in hospital with a severe condition and it will probably take a few weeks until my health is fully restored again. I hope it's ok if I'll take my time and will come back to you with the adapted PRs then :)
ismayilov-ismayil
commented
1 year ago Hi @echalone. No worries, get well soon!
ismayilov-ismayil
commented
1 year ago Hi @echalone I hope everything is going well for you. Did you have a chance to complete this PR?
echalone
commented
1 year ago Hello @ismayilov-ismayil Thank you, I'm back for a few weeks now though I was out for a few months. So I'm rather busy with the most pressing topics in our company right now but am planning to come back to this as soon as possible, hopefully in the next few weeks :)
echalone
commented
1 year ago Hello @ismayilov-ismayil, according to your suggestion I have now recreated the pull request for these changes. It previously contained other changes too and in the new pull request these changes have been isolated and now only contain the changes needed to fix this bug. New pull request is here: https://github.com/microsoft/azure-pipelines-agent/pull/4216 Thank you for your suggestion.
I just hope we don't have to start validation or testing from the start again with those new pull requests and they won't get queued to the very end, I'm already waiting some time (nearly 2 years now) for this to be included ^^
github-actions[bot]
commented
10 months ago This issue has had no activity in 180 days. Please comment if it is not actually stale
echalone
commented
10 months ago not stale
github-actions[bot]
commented
4 months ago This issue has had no activity in 180 days. Please comment if it is not actually stale
echalone
commented
3 months ago Once upon a time, in the kingdom of the azure-pipeline-agent repository, there lurked a mischievous creature known as the GitHub Stale Bot. This bot had a peculiar habit of labeling open pull requests and feature requests with a "stale" label after half a year of inactivity. And if that wasn't enough, it would ruthlessly close these issues after another week if nobody dared to utter a word in their defense. The developers of the azure-pipeline-agent repository lived in constant fear of this relentless bot. They would toil away at their keyboards, creating magnificent lines of code, only to have their creations labeled as "stale" and cast aside into the abyss of forgotten tickets. But one day, those brave developers stood up to challenge of the GitHub Stale Bot. Armed with nothing but their wit and a keyboard they devised a cunning plan to outsmart the bot once and for all. As the deadline approached, and passed, for the bot to strike, we sprung into action. We crafted a comment so clever, so witty, that even the most heartless of bots would have to pause and reconsider its actions. The comment echoed through the digital halls of GitHub, reaching the circuits of the Stale Bot's mechanical heart. And to nobody's surprise, the bot replied: "removed the stale label" From that day forward, we became a legend in the kingdom of the azure-pipeline-agent repository. Whenever the GitHub Stale Bot threatened to strike, developers would flock to us for advice on crafting the perfect comment to save their creations.
This is that comment. This is our story.
UnitTest localization problems
Five UnitTests have problems on some non-english systems (at least in German, French, Italian and Spanish) and will fail although they shouldn't. This is due to differently named system accounts in different languages and different decimal separators in different language cultures.
I would have already fixed the problem in those UnitTests and have already created a pull request to the master branch of this repository: https://github.com/microsoft/azure-pipelines-agent/pull/4216 The pull request needs to be reviewed and accepted.
Failing UnitTests
Microsoft.VisualStudio.Services.Agent.Tests.Listener.Configuration.ArgumentValidatorTestsL0.WindowsLogonAccountValidator Fails on some non-english Windows systems with message: Expected: True/Actual: False; Assert.True() Failure Reason: On some non-english Windows systems (German, French, Italian and Spanish) the local service account is named differently, this should be taken into account (pun intended). The different account names are:
Test.L0.Listener.Configuration.NativeWindowsServiceHelperL0.EnsureGetDefaultServiceAccountShouldReturnNetworkServiceAccount Fails on some non-english Windows systems with message: Expected: True/Actual: False; If agent is getting configured as build-release agent, default service accout should be 'NT AUTHORITY\NETWORK SERVICE' Reason: On some non-english Windows systems (German, French, Italian and Spanish) the network service account is named differently, this should be taken into account. The different account names are:
Test.L0.Listener.Configuration.NativeWindowsServiceHelperL0.EnsureGetDefaultAdminServiceAccountShouldReturnLocalSystemAccount Fails on some non-english Windows systems with message: Expected: True/Actual: False; If agent is getting configured as deployment agent, default service accout should be 'NT AUTHORITY\SYSTEM' Reason: On some non-english Windows systems (German and French) the system account is named differently, this should be taken into account. The different account names are:
Microsoft.VisualStudio.Services.Agent.Tests.Worker.CodeCoverage.CoberturaSummaryReaderTests.VerifyCoberturaCoverageStatisticsForValidSummaryFile Fails on some non-english Windows systems with message: Expected: 2/Actual: 24; Assert.Equal() Failure Reason: On some non-english Windows systems (for example: German) the decimal separator in numbers isn't a point (but, for example, a comma). However, the hard coded test xml uses a point as the decimal separator for all numbers. It is assumed that the coverage tool will generate an xml with decimal separators according to the culture of the installed system, so the test xml should be adapted according to the culture of the installed system as well. The UnitTest should work fine after this adaption as the code already handles it that way.
Microsoft.VisualStudio.Services.Agent.Tests.Worker.TestResults.XUnitResultReaderTests.ReadResultsReturnsCorrectValues Fails on some non-english Windows systems with message: 1042.2319/Actual: 1042,2319; Assert.Equal() Failure Reason: On some non-english Windows systems (for example: German) the decimal separator in numbers isn't a point (but, for example, a comma). However, the hard coded test xml uses a point as the decimal separator for all numbers. It then generates strings out of numbers and compares those to the values read in from the test xml. However, those then might differ if the culture of the installed system does not use a point as the decimal separator. To be sure to generate the correct string out of numbers so the comparison won't fail we have to specifically state which decimal separator (the point) we want to use when generating a string out of numbers in the UnitTest.
Agent Version and Platform
Latest master source code (since it's UnitTests) or latest version v2.189.0 of the agent. OS of the machine running the UnitTests: Windows Language of machine running the UnitTests: German, French, Italian, Spanish (potentially other languages if decimal separator isn't a point in those languages)
UnitTest output
[xUnit.net 00:00:02.9401910] Discovering: Test [xUnit.net 00:00:03.4152420] Discovered: Test [xUnit.net 00:00:03.5566454] Starting: Test [xUnit.net 00:00:04.3653681] If agent is getting configured as deployment agent, default service accout should be 'NT AUTHORITY\SYSTEM' [xUnit.net 00:00:04.3657665] Expected: True [xUnit.net 00:00:04.3660228] Actual: False [xUnit.net 00:00:04.3725354] Stack Trace: [xUnit.net 00:00:04.3737561] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Listener\Configuration\NativeWindowsServiceHelperL0.cs(54,0): at Test.L0.Listener.Configuration.NativeWindowsServiceHelperL0.EnsureGetDefaultAdminServiceAccountShouldReturnLocalSystemAccount() [xUnit.net 00:00:04.4199174] If agent is getting configured as build-release agent, default service accout should be 'NT AUTHORITY\NETWORK SERVICE' [xUnit.net 00:00:04.4201525] Expected: True [xUnit.net 00:00:04.4202063] Actual: False [xUnit.net 00:00:04.4203190] Stack Trace: [xUnit.net 00:00:04.4204858] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Listener\Configuration\NativeWindowsServiceHelperL0.cs(36,0): at Test.L0.Listener.Configuration.NativeWindowsServiceHelperL0.EnsureGetDefaultServiceAccountShouldReturnNetworkServiceAccount() [xUnit.net 00:00:04.5100850] Assert.Equal() Failure [xUnit.net 00:00:04.5101459] ↓ (pos 4) [xUnit.net 00:00:04.5104088] Expected: 1042.2319 [xUnit.net 00:00:04.5104636] Actual: 1042,2319 [xUnit.net 00:00:04.5105159] ↑ (pos 4) [xUnit.net 00:00:04.5106731] Stack Trace: [xUnit.net 00:00:04.5107625] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Worker\TestResults\Legacy\XUnitResultReaderTests.cs(215,0): at Microsoft.VisualStudio.Services.Agent.Tests.Worker.TestResults.XUnitResultReaderTests.ReadResultsReturnsCorrectValues() [xUnit.net 00:00:05.8140472] Assert.Equal() Failure [xUnit.net 00:00:05.8141035] Expected: 2 [xUnit.net 00:00:05.8141383] Actual: 24 [xUnit.net 00:00:05.8141845] Stack Trace: [xUnit.net 00:00:05.8142493] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Worker\CodeCoverage\CoberturaSummaryReaderTests.cs(167,0): at Microsoft.VisualStudio.Services.Agent.Tests.Worker.CodeCoverage.CoberturaSummaryReaderTests.VerifyBranchCoverageStats(List`1 coverageStats) [xUnit.net 00:00:05.8143303] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Worker\CodeCoverage\CoberturaSummaryReaderTests.cs(40,0): at Microsoft.VisualStudio.Services.Agent.Tests.Worker.CodeCoverage.CoberturaSummaryReaderTests.VerifyCoberturaCoverageStatisticsForValidSummaryFile() [xUnit.net 00:00:06.4075994] Assert.True() Failure [xUnit.net 00:00:06.4076571] Expected: True [xUnit.net 00:00:06.4077016] Actual: False [xUnit.net 00:00:06.4077498] Stack Trace: [xUnit.net 00:00:06.4078355] D:\GitHub\echalone\azure-pipelines-agent\src\Test\L0\Listener\Configuration\ArgumentValidatorTestsL0.cs(59,0): at Microsoft.VisualStudio.Services.Agent.Tests.Listener.Configuration.ArgumentValidatorTestsL0.WindowsLogonAccountValidator() [xUnit.net 00:00:16.8175986] Finished: Test
PS: I'm reopening/readding this issue since https://github.com/microsoft/azure-pipelines-agent/issues/3481 was wrongfully closed by a stale-bot, it's not really stale, it just hasn't been processed yet