Revert breaking change when packing files and using forward slashes #4270
Conversation
jeffkl
force-pushed
the
dev-jeffkl-fix-unflattened-files-in-packagebuilder
branch
from
4ad7c20
to
57d0235
Compare
| @@ -6318,7 +6318,7 @@ public void PackCommand_PackReadme_DirSeparatorForwardSlash1_Succeeds() | |||
| TestPackReadmeSuccess(testDir, @"docs/readme.md"); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [Fact(Skip = "https://github.com/NuGet/Home/issues/11234")] | |||
There was a problem hiding this comment.
These tests are "broken" by this revert
| [PlatformTheory(Platform.Windows)] | ||
| [InlineData(@"dir1\dir2\**", true, "Content")] | ||
| [InlineData("dir1/dir2/**", false, "Content")] | ||
| public void PackageBuilder_AddFiles_HasDifferentBehaviorDependingOnSlash(string source, bool expectFlattened, string destination) |
There was a problem hiding this comment.
This test ensures the breaking change will be caught next time
| { | ||
| new | ||
| { | ||
| Path = expectFlattened ? $"Content{Path.DirectorySeparatorChar}file1.txt" : $"Content{Path.DirectorySeparatorChar}dir1{Path.DirectorySeparatorChar}dir2{Path.DirectorySeparatorChar}file1.txt", |
There was a problem hiding this comment.
Isn't this else dependent on knowing the input to the test? That seems like not a "best practice".
There was a problem hiding this comment.
Yeah its how I designed the test, I pass in a path with / and expect the files to be flattened and then paths with \ and expect them to not be flattened. I could do two separate tests but I foresee us fixing this "bug" and making them return the same things so this test would be updated to remove these ternary operators. Do you think there's a better way to accomplish this?
There was a problem hiding this comment.
Maybe I'm not fully thinking about it, but I would think you'd pass in both the unflattened string and the flattened string to the test, that way you assert the 2 test inputs match after running through the method.
There was a problem hiding this comment.
I'm going to merge it as-is for now, but I will update it later. It used to need to run on all platforms which is why I tried to make it so dynamic. But it ended up only needing to be run on Windows so it could be made to have a constant value for expected. I'll just make that change when this whole feature is fixed.
test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageBuilderTest.cs
Outdated
Show resolved
Hide resolved
jeffkl
deleted the
dev-jeffkl-fix-unflattened-files-in-packagebuilder
branch
Bug
Fixes: NuGet/Home#11125
Regression? Yes Last working version: 5.9.1
Description
Our code has different behavior if you use a back slash versus a forward slash during pack. The feature to pack a README "fixed" this bug but regressed another.
I've created NuGet/Home#11234 to track the fact that the behavior is different and this PR includes a unit test to catch if we regress it again.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation