Fix test reliability issues with checks in GetObjectRoot#179
Conversation
wilbaker
added
the
type: test-reliability
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
My overall feedback is that this test was too hard to decipher. The logic all makes sense now, but it took a lot of work to figure out what the intent was.
Consider reworking it to something more like:
this.LocalCacheRoot.ShouldBeADirectory(fileSystem).WithFile("mapping.dat");
this.LocalCacheRoot.ShouldBeADirectory(fileSystem).WithNoFilesExcept("mapping.dat", "mapping.dat.lock");
| FileInfo[] files = localCacheRootItems.Where(info => info is FileInfo).Cast<FileInfo>().ToArray(); | ||
| files.Where(f => string.Equals(f.Name, "mapping.dat", StringComparison.OrdinalIgnoreCase)) | ||
| .Count() | ||
| .ShouldEqual(1, "Local cache root should contain a single 'mapping.dat' file, actual files: " + string.Join<FileInfo>(",", files)); |
There was a problem hiding this comment.
Nit: use a tab size of 4 spaces
| .ShouldEqual(1, "Local cache root should contain a single 'mapping.dat' file, actual files: " + string.Join<FileInfo>(",", files)); | ||
|
|
||
| IEnumerable<FileInfo> unexpectedFiles = files.Where( | ||
| f => !string.Equals(f.Name, "mapping.dat", StringComparison.OrdinalIgnoreCase) && |
There was a problem hiding this comment.
The spacing here is a bit hard to parse. Maybe move the f => to the previous line, so the two parts of the condition can line up better?
| FileInfo[] files = localCacheRootItems.Where(info => info is FileInfo).Cast<FileInfo>().ToArray(); | ||
| files.Where(f => string.Equals(f.Name, "mapping.dat", StringComparison.OrdinalIgnoreCase)) | ||
| .Count() | ||
| .ShouldEqual(1, "Local cache root should contain a single 'mapping.dat' file, actual files: " + string.Join<FileInfo>(",", files)); |
There was a problem hiding this comment.
This count can only ever be 1 or 0 right? This looks like an indirect way to check fileSystem.FileExists(Path.Combine(this.LocalCacheRoot, "mapping.dat")).ShouldBeTrue(). Or we could stick to the other pattern with this.LocalCacheRoot.ShouldBeADirectory(fileSystem).WithFile("mapping.dat"). I think either of those would make the intent of the test more clear.
There was a problem hiding this comment.
Good point, I got caught up in how the check was working before. It's much cleaner to use the helpers we already have.
| f => !string.Equals(f.Name, "mapping.dat", StringComparison.OrdinalIgnoreCase) && | ||
| !string.Equals(f.Name, "mapping.dat.lock", StringComparison.OrdinalIgnoreCase)); | ||
|
|
||
| unexpectedFiles.Any().ShouldBeFalse("Local cache root contains unexpected files: " + string.Join(",", unexpectedFiles)); |
There was a problem hiding this comment.
I don't understand how this solves the test failure. The original code was failing because we found 3 items in the cache root instead of 2, but this code is still going to fail if there are more than the 2 expected items.
| IEnumerable<FileSystemInfo> localCacheRootItems = this.LocalCacheRoot.ShouldBeADirectory(fileSystem).WithItems(); | ||
| localCacheRootItems.Count().ShouldEqual(2, "Expected local cache root to contain 2 items. Actual items: " + string.Join(",", localCacheRootItems)); | ||
|
|
||
| FileInfo[] files = localCacheRootItems.Where(info => info is FileInfo).Cast<FileInfo>().ToArray(); |
There was a problem hiding this comment.
Ah I missed that you were filtering down to just files here. You could consider changing line 117 above to this.LocalCacheRoot.ShouldBeADirectory(fileSystem).WithFiles() and have that return just FileInfos to begin with.
| .Count() | ||
| .ShouldEqual(1, "Local cache root should contain a single 'mapping.dat' file, actual files: " + string.Join<FileInfo>(",", files)); | ||
|
|
||
| IEnumerable<FileInfo> unexpectedFiles = files.Where( |
There was a problem hiding this comment.
This looks fine to me, but you might consider creating an array (or maybe HashSet<string>, although it doesn't make a practical difference) of the list of expected file names up front, then you could simplify the LINQ statement to something like:
HashSet<string> expectedFiles = new HashSet<string>(StringComparer.OrdinalIgnoreCase) {"expected_file1", "expectedFile2"};
unexpectedFiles.Where(f => expectedFile.Contains(f.Name));
| { | ||
| T item = group.SingleOrDefault(predicate); | ||
| item.ShouldEqual(default(T)); | ||
| item.ShouldEqual(default(T), "Unexpected matching entry found in {" + string.Join(",", group) + "}"); |
There was a problem hiding this comment.
I think the body of this method could be simplified to:
group.Any(predicate).ShouldEqual(false, "Unexpected matching entry found in {" + string.Join(",", group) + "}");
There was a problem hiding this comment.
I think I'll leave this as-is for now and keep the changes focused on fixing the functional test reliability issue (I do agree this is a nice potential cleanup, but would rather not use this PR for refactoring\cleaning the Should helpers)
|
@wilbaker could we also port this back to milestones/M139? |
|
@jrbriggs sure I'll port this over |
4 participants
Fix for issue #178
The problem with the current
GetObjectRootis the check there are only two items in the local cache root folder.This was a valid check when the functional tests were not run in parallel, but now that they are run in parallel it's possible that in one test GVFS is reading
mapping.dat(which requires creating\holdingmapping.dat.lock) while another test is callingGetObjectRoot(which doesn't need to readmapping.datbut still wants to validate the integrity of the local cache root folder).To make these checks more robust
GetObjectRootwill now check for the specific files it expects (and does not expect) to be present.Example of a test failing due to this issue:
18227.3