Path tests overhaul #27449
Path tests overhaul #27449
Conversation
Path tests weren't running due to a test infra bug (too many test methods in one class apparently). Cleaned up path tests, fixing issues and adding coverage.
| Assert.Equal(expected, Path.GetFileName(path)); | ||
| } | ||
|
|
||
| [ActiveIssue(27269, TargetFrameworkMonikers.Netcoreapp)] |
There was a problem hiding this comment.
No, unfortunately. I'm not sure why we don't have a new CoreLib, I've run out of mental bandwidth to figure it out. Once we do, we can just remove these few and everything should pass. Well, with dotnet/coreclr#16554 anyway. :)
|
CoreCLR build is broken so no insertion. https://github.com/dotnet/core-eng/issues/2753 |
|
I do not think we should be working around the bug by refactoring the tests. The bug is likely affecting random other places. We need to get that bug traced down and fixed fast. |
|
E.g. I see that |
I needed to fix the tests anyway- they were broken and scenarios were missing/incorrect. It was an 8+ hour effort to get them corrected for both the currently ingested bits and the pending CoreCLR changes from the past few days. They are also better factored for me to make additional passes to beef up the coverage. It is good we now know that we've got a runtime problem and it is reproing in multiple ways. Should we track with #27426 or open another issue in CoreCLR? |
| @@ -1,4 +1,3 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
|
|
||
| namespace System.IO.Tests | ||
| { | ||
| public partial class PathTests_Windows : PathTestsBase |
There was a problem hiding this comment.
If this refactoring is permanent, the name of the file should be PathTests_Windows to follow the naming conventions.
| { | ||
| public static class TestHelpers | ||
| { | ||
|
|
| namespace System.IO.Tests | ||
| { | ||
| public static partial class PathTests | ||
| public class PathTests_Combine |
There was a problem hiding this comment.
Nit: file name vs. class name mismatch
| { | ||
| [PlatformSpecific(TestPlatforms.Windows)] | ||
| [SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework)] | ||
| public class PathTests_NetFX : PathTestsBase |
There was a problem hiding this comment.
Nit: File name vs class name mismatch
There was a problem hiding this comment.
This one is a bit awkward. I really want to keep it in the build as it makes it much easier to work with, but I don't want to litter the file with [SkipOnTargetFramework] and yet still clearly show the relationship when browsing the solution explorer. :/
|
@dotnet/dncenghot It appears that the Ubuntu.1804.Amd64.Open runs are hung up? All of the Linux x64 Release Builds are spinning since last night. |
|
1804 is jammed on other PR's also. |
|
@danmosemsft Should we skip on 1804 if all other leg results are good? |
|
@JeremyKuhne let's see whether it can be quickly fixed tomorrow morning. |
We don't have much time left in the month. :/ Note that this one will be necessary to pass the next CoreCLR ingestion attempt. |
|
Ugh..OK, skipping it here seems reasonable given that fact and that is new and we have other Ubuntu coverage. |
Thanks :) |
|
@JeremyKuhne looking at the Ubuntu 18.04 queue now. I think the problem is simply that the images expire faster than we'd like as it's not RTM yet. |
|
@JeremyKuhne & @danmosemsft : This is fixed now. Summary: Since Ubuntu 18 is not RTMed yet and we run a daily build, they update their image VERY often and we "fell off". We lock in our image version to prevent random change taking machines down which did not help in this circumstance. I've moved our VMs to set to "latest" version (which has caused problems in the past when latest = broken build) until a release version 18.04 is finalized to prevent this going forward. |
|
Thanks @MattGal ! |
* Path tests overhaul Path tests weren't running due to a test infra bug (too many test methods in one class apparently). Cleaned up path tests, fixing issues and adding coverage. * Address feedback * More name tweaks Commit migrated from dotnet/corefx@eb39a50
5 participants
Path tests weren't running due to a test infra or runtime bug (too many test methods in one class apparently). Cleaned up path tests, fixing issues and adding coverage.
cc: @danmosemsft, @Anipik, @maryamariyan, @pjanotti
See dotnet/coreclr#16554, #27426