Fix failing unit tests in Arcade SDK#7159
Conversation
|
There were no reviews yet so I pushed another part of fixes for |
|
Added fixes for |
riarenas
left a comment
There was a problem hiding this comment.
The changes seem reasonable but I'm not familiar with the components under test enough to know if these tests are still correct.
Can you include a commit to enable these tests during PRs? I don't think we should merge this change until we're sure that they run during builds, otherwise they're just going to get out of date again and break.
|
@riarenas Yep, that's a good idea. There are still some tests that I didn't fix yet but I can mark them as skipped with comment pointing to the related issue. They are not passing anyway at the moment and it should allow us to run them in CI builds. |
|
I pushed commit where I enable Arcade.SDK.Tests project and the build was successful. |
riarenas
left a comment
There was a problem hiding this comment.
Thanks for adding the issue to the skipped tests and enabling them again. Should be easier to determine whether a test needs attention or not since they are actually running somewhere now.
…dotnet#7159) This commit re-enables `Microsoft.DotNet.Arcade.Sdk.Tests` project when during Helix test runs. Previously it was disabled because there were some problems with test environment on Helix. Additionally it fixes the following test failures: ### FindLatestDropTests `JObject.Parse` parses JSON dates as `DateTime` by default not as `string`. When we were calling `(string)item["CreatedDateUtc"]` the `DateTime` stored in that property was converted to string using invariant culture and then we tried to parse it back using current culture. It worked correctly as long as the date time formats for both cultures were the same but this is only true for `en-US`. I also converted `GetLatestDropName_Error` test to use `Theory` instead of having four asserts inside. This makes it easier to see which case actually failed. ### GenerateResxSourceTests The generation code was changed some time ago but the test data that we use as the expected output was not updated. I copied the current output of the generator and put it into test assets. Of course this assumes that the current output is correct but it looks like it is. ### GenerateSourcePackageSourceLinkTargetsFileTests The tests failed on Unix because some of the backslashes in test strings were converted to front slashes but not all. It was fixed by explicitly normalizing all paths before passing them to the tested code. ### MinimalRepoTests / RepoWithConditionalProjectsToBuildTests I had to mark these tests as skipped because they are not trivial to fix and they block us from enabling Arcade.SDK.Tests on CI builds. We should enable them back as soon as they are fixed. Related issue: dotnet#7092 ### To double check: * [X] The right tests are in and and the right validation has happened. Guidance: https://github.com/dotnet/core-eng/tree/master/Documentation/Validation
FindLatestDropTests
This PR fixes culture-sensitive date parsing in
FindLatestDroptask.JObject.Parseparses JSON dates asDateTimeby default not asstring. When we were calling(string)item["CreatedDateUtc"]theDateTimestored in that property was converted to string using invariant culture and then we tried to parse it back using current culture. It worked correctly as long as the date time formats for both cultures were the same but this is only true foren-US.I also converted
GetLatestDropName_Errortest to useTheoryinstead of having four asserts inside. This makes it easier to see which case actually failed.GenerateResxSourceTests
The generation code was changed some time ago but the test data that we use as the expected output was not updated. I copied the current output of the generator and put it into test assets. Of course this assumes that the current output is correct but it looks like it is.
GenerateSourcePackageSourceLinkTargetsFileTests
The tests failed on Unix because some of the backslashes in test strings were converted to front slashes but not all. It was fixed by explicitly normalizing all paths before passing them to the tested code.
MinimalRepoTests / RepoWithConditionalProjectsToBuildTests
I had to mark these tests as skipped because they are not trivial to fix and they block us from enabling Arcade.SDK.Tests on CI builds. We should enable them back as soon as they are fixed.
Related issue: #7092
To double check: