Skip to content

[Linux] Revise functional tests for cross-device mounts#1413

Merged
chrisd8088 merged 2 commits intomicrosoft:masterfrom
github:posix-and-crossdevice-tests
Sep 24, 2019
Merged

[Linux] Revise functional tests for cross-device mounts#1413
chrisd8088 merged 2 commits intomicrosoft:masterfrom
github:posix-and-crossdevice-tests

Conversation

@chrisd8088
Copy link
Contributor

@chrisd8088 chrisd8088 commented Aug 5, 2019 •
edited
Loading

On POSIX systems, where a rename(2) returns EXDEV when trying to move a file or directory between mounted filesystems, the functional tests which do this currently fail if the
repository is mounted on a separate device (as is the case on Linux).

Therefore we create a RepositoryMountsSameFileSystem test category for use on VFSForGit implementations which allow renaming across repository boundaries (i.e., Windows PrjFlt
and Mac kext), and mark a number of tests with this category designation as they move files or folders in or out of the repository, or create hard links across the repository boundary.

In the ModifiedPathsTests suite, two specific tests which check the contents of the modified-paths database after remounting and after hard-linking, specifically, are subdivided into pairs of tests. Each pair consists of one test which only checks changes that do not cross the repository boundary, suitable for all implementations, and one test which check changes spanning the repository boundary and which is marked with the RepositoryMountsSameFileSystem
category.

@chrisd8088 chrisd8088 added the WIP label Aug 5, 2019
@chrisd8088 chrisd8088 changed the title [WIP] Revise functional tests for POSIX and cross-device mounts [POSIX] Revise functional tests for POSIX and cross-device mounts Aug 5, 2019
@chrisd8088 chrisd8088 changed the title [POSIX] Revise functional tests for POSIX and cross-device mounts [Linux] Revise functional tests for POSIX and cross-device mounts Aug 5, 2019
@chrisd8088 chrisd8088 marked this pull request as ready for review August 5, 2019 13:04
This was referenced Aug 8, 2019
Merged
Closed
Closed
@chrisd8088 chrisd8088 force-pushed the posix-and-crossdevice-tests branch from 58afa47 to facd52f Compare August 8, 2019 06:26
@chrisd8088 chrisd8088 force-pushed the posix-and-crossdevice-tests branch 2 times, most recently from b4640b8 to 104b1a2 Compare August 23, 2019 16:47
wilbaker
wilbaker reviewed Aug 27, 2019
View reviewed changes
Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished a first pass over the changes.

I have not left any comments on ModifiedPathsTests.cs yet as I'd like to hear your thoughts on only adding a RepositoryMountsSameFileSystem category and having the "copy+delete" (i.e. bash mv) tests run on all filesystems.

@chrisd8088 chrisd8088 force-pushed the posix-and-crossdevice-tests branch 3 times, most recently from c26e49f to 8d132b0 Compare September 10, 2019 05:32
@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 10, 2019 •
edited
Loading

@wilbaker -- I think I've managed to simplify this so that we just ignore the tests which don't apply if cross-device renaming (and hard-linking) are not supported; I can't, on reflection, see much value in emulating them by copying files. The exceptions, sort of, are the two main tests in ModifiedPathsTests, which I split up as described into one each which makes inside-repo-only changes, and one which makes repo-boundary-crossing changes (and can thus be excluded on Linux).

@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 20, 2019

Just as an added thought, @wilbaker, I can also submit these changes as a series of smaller PRs -- e.g., one each for Mac -> POSIX renaming, adding FileSystemSupportsFileMode, and adding RepositoryMountsSameFileSystem. I'd be happy to do so if that would make your life easier! :-)

@wilbaker
Copy link
Member

wilbaker commented Sep 20, 2019

I can also submit these changes as a series of smaller PRs -- e.g., one each for Mac -> POSIX renaming, adding FileSystemSupportsFileMode, and adding RepositoryMountsSameFileSystem. I'd be happy to do so if that would make your life easier! :-)

@chrisd8088 if you don't mind making that change I think it would be easier/quicker to review these changes in chunks. Thanks!

@chrisd8088 chrisd8088 force-pushed the posix-and-crossdevice-tests branch from 8d132b0 to 969b6b0 Compare September 23, 2019 20:16
@chrisd8088 chrisd8088 changed the title [Linux] Revise functional tests for POSIX and cross-device mounts [Linux] Revise functional tests for cross-device mounts Sep 23, 2019
@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 23, 2019

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 23, 2019

No pipelines are associated with this pull request.

@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 24, 2019

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 24, 2019

No pipelines are associated with this pull request.

@chrisd8088 chrisd8088 mentioned this pull request Sep 24, 2019
Merged
@chrisd8088 chrisd8088 closed this Sep 24, 2019
@chrisd8088 chrisd8088 reopened this Sep 24, 2019
@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 24, 2019

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 24, 2019

No pipelines are associated with this pull request.

@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 24, 2019

/azp run GitHub VFSForGit Mac Functional Tests

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 24, 2019

No pipelines are associated with this pull request.

@chrisd8088
Copy link
Contributor Author

chrisd8088 commented Sep 24, 2019 •
edited
Loading

OK @wilbaker -- after much duelling with some hanging CI tests, this is ready again, after being trimmed down to just the cross-device changes. The POSIX-related ones I moved over to #1529.

wilbaker
wilbaker suggested changes Sep 24, 2019
View reviewed changes
Copy link
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the changes look good, I am having trouble seeing where we're including/excluding RepositoryMountsSameFileSystem tests with these changes.

@chrisd8088
separate func tests which rename in/out of repo
b6d08da 
On POSIX, where a rename(2) returns EXDEV when trying
to move a file or directory between mounted filesystems,
the functional tests which do this currently fail if the
repository is mounted on a separate device (as is the case
on Linux).

Therefore we create a RepositoryMountsSameFileSystem test
category for use on VFSForGit implementations which allow
rename(2) across repository boundaries (i.e., Windows PrjFlt
and Mac kext), and mark a number of tests which move contents
in or out of the repository with this category designation.

In the ModifiedPathsTests suite, two specific tests which
check the contents of the ModifiedPaths database after
remounting and after hard-linking, specifically, are
subdivided into pairs of tests.  Each pair consists of one
test which only checks changes that do not cross the
repository boundary, suitable for all implementations, and one
test which check changes spanning the repository boundary and
which is marked with the RepositoryMountsSameFileSystem
category.
@chrisd8088
tidy same-filesystem func test category comments
d28ed04 
Per PR advice from wilbaker, we avoid sprinkling comments
throughout the functional test suite regarding the use of
the RepositoryMountsSameFileSystem category, and instead
add a more verbose description in Categories.cs which
should clarify the expected use case for this test flag.
@chrisd8088 chrisd8088 force-pushed the posix-and-crossdevice-tests branch from 969b6b0 to d28ed04 Compare September 24, 2019 18:23
@chrisd8088 chrisd8088 merged commit fd2551c into microsoft:master Sep 24, 2019
@chrisd8088 chrisd8088 deleted the posix-and-crossdevice-tests branch September 24, 2019 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derrickstolee derrickstolee Awaiting requested review from derrickstolee

1 more reviewer

@wilbaker wilbaker wilbaker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects: tests platform: linux

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chrisd8088 @wilbaker