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

VerifyDirectory doesn't play well with file names starting with dot #699

Closed
JanKrivanek opened this issue Nov 3, 2022 · 4 comments
Closed
Labels
Milestone

Comments

@JanKrivanek
Copy link
Contributor

JanKrivanek commented Nov 3, 2022

Problem

VerifyDirectory has 2 issues with file names starting with dot (e.g. .editorconfig):

  • When file starting with dot is in the root of directory to be verified, verifier gets an exception (since file name is resolved to empty string which is disallowed):
System.ArgumentNullException
Value cannot be null. (Parameter 'name')
   at Guard.AgainstEmpty(String value, String argumentName) in C:\src\Verify\src\Verify\Guard.cs:line 108
   at VerifyTests.Target..ctor(String extension, StringBuilder data, String name) in C:\src\Verify\src\Verify\Target.cs:line 57
   at InnerVerifier.ToTargets(String path, Func`2 include, IEnumerable`1 enumerateFiles, Object info, FileScrubber fileScrubber)+MoveNext() in C:\src\Verify\src\Verify\Verifier\InnerVerifier_Directory.cs:line 104
   at InnerVerifier.ToTargets(String path, Func`2 include, IEnumerable`1 enumerateFiles, Object info, FileScrubber fileScrubber)+System.Threading.Tasks.Sources.IValueTaskSource<System.Boolean>.GetResult()
   at Extensions.ToList[T](IAsyncEnumerable`1 target) in C:\src\Verify\src\Verify\Extensions.cs:line 6
   at Extensions.ToList[T](IAsyncEnumerable`1 target) in C:\src\Verify\src\Verify\Extensions.cs:line 6
   at InnerVerifier.VerifyDirectory(String path, Func`2 include, String pattern, EnumerationOptions option, Object info, FileScrubber fileScrubber) in C:\src\Verify\src\Verify\Verifier\InnerVerifier_Directory.cs:line 20
   at VerifyXunit.Verifier.<>c__DisplayClass6_0.<<Verify>b__0>d.MoveNext() in C:\src\Verify\src\Verify.Xunit\Verifier.cs:line 64

Root cause

For the first case it's inability to work with empty string as a name. Can be solved by using null instead of empty string - code will then fallback to NameOrTarget.

For the second case - I'm not sure. I'm blocked by the WSL debugging issue #654 (that's caused by the fact that 'CallerFilePathAttribute' returns paths in windows FS (as that's where binaries were built) - so Path utilities and locating of snapshot files fails to work properly) - so I haven't got a chance to debug this properly (I'd need to setup full build env and remote debugger in my WSL - for which I'll need some extra time).

Possible solution

Both issues can be likely fixed by using a dummy name (e.g. 'trarget') as a filename in case the name resolves to empty string or directory name within the InnerVerifier.ToTargets (with name for nested file would still need to contain the relative dir as today - e.g. nested_dir\target). This however feels a bit ugly (artificial filename).

@JanKrivanek
Copy link
Contributor Author

This was an interesting one. Rootcause is in subtle different ways of enumerating files.

Snapshot files are enumerated via EnumerateFiles using SearchOptions (https://github.com/VerifyTests/Verify/blob/main/src/Verify/IoHelpers.cs#L33)
The files to be verified are enumerated via EnumerateFiles using EnumerationOptions (https://github.com/VerifyTests/Verify/blob/main/src/Verify/Verifier/InnerVerifier_Directory.cs#L18)

The Directory.EnumerateFiles with SearchOptions are enriched (in runtime) with few extra flags (https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/IO/EnumerationOptions.cs#L24) - specifically the AttributesToSkip are cleared.

This means that the caller of VerifyDirectory can workaround this issue via passing the EnumerationOptions explicitly. Though the discrepancy in enumerating still feels as a bug - I'll stitch together a fix proposal.

In order to debug more comfortably under WSL I put together simple mapping of paths - I'll post a draft of that as well (it just needs to be called from all usages of CallerFilePathAttribute - which there are many - so In a first version I'll post just a sample if the idea itself looks OK).

@SimonCropp
Copy link
Member

so as you pointed out there were a few moving pieces here.

i went with prefixing with nofilename. thoughts?

i deployed a 18.3.1-beta.1 if you want to play with it

@SimonCropp SimonCropp added the Bug label Nov 3, 2022
@JanKrivanek
Copy link
Contributor Author

Using nofilename serves the purpose and resolves the issue. Analogous to noextension.

It would be nice if original filename could be preserved (regardles of filename or extension) - but it's definitely not a blocker and current solution is clean and simple from code perspective.

I'll bump our reference. Thanks!

@SimonCropp
Copy link
Member

It would be nice if original filename could be preserved

hmmm. i will see how difficult that is

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

No branches or pull requests

2 participants