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

tests: remove mount_id and parent_id from mount-ns test data #7448

Merged
merged 1 commit into from Sep 12, 2019

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Sep 11, 2019

The mount-ns test is useful as it shows a realistic view of the mount
table. The mount IDs are re-numbered and are pretty much just a
sequence. While some work went into making it easy to understand which
mount namespace we are looking at, by checking the mount identifier
value for membership in ranges 0..999, 1000..1999 or 2000..2999 - the
overall value comes at a great cost. Any insertions or removals cause
massive re-numbering to occur below the change. This makes it difficult
to evaluate differences that a patch is introducing without manually
convincing oneself of the "real" diff buried inside the diff printed by
the tooling.

This patch discards the first two columns, namely mount_id and parent_id
in the hope that the resulting output is clearer and more useful.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

@zyga
Copy link
Collaborator Author

zyga commented Sep 11, 2019

This branch includes #7446 which is opened for separate review.

@anonymouse64
Copy link
Member

In general, I think this change would be really helpful in looking at the diff between different mount tables, however the one potential concern I have is that if a mount was unmounted then remounted, then IIUC the mount_id and parent_id would change but nothing else would, so we would miss this change. OTOH, perhaps this is very unlikely to happen in practice and as such is okay to drop the mount_id and parent_id from the output. Also perhaps we are already missing this case since we re-number the mounts anyways so 🤷‍♂️

Overall +1 on this idea though - I like 1-line diffs 😃

@zyga
Copy link
Collaborator Author

zyga commented Sep 11, 2019

The problem of not detecting things being unmounted and then mounted back again is valid but not applicable to this specific test. The test only measures static properties. The regression test in #7436 handles some of the dynamic properties. In addition as you observed the renumbering does make the mount_id and parent_id relatively useless since they are altered for test-to-test determinism.

Thank you for the +1. I will work towards making this pull request mergeable.

The mount-ns test is useful as it shows a realistic view of the mount
table. The mount IDs are re-numbered and are pretty much just a
sequence. While some work went into making it easy to understand which
mount namespace we are looking at, by checking the mount identifier
value for membership in ranges 0..999, 1000..1999 or 2000..2999 - the
overall value comes at a great cost. Any insertions or removals cause
massive re-numbering to occur below the change. This makes it difficult
to evaluate differences that a patch is introducing without manually
convincing oneself of the "real" diff buried inside the diff printed by
the tooling.

This patch discards the first fro columns, namely mount_id and parent_id
in the hope that the resulting output is clearer and more useful.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga zyga marked this pull request as ready for review September 12, 2019 06:47
@zyga
Copy link
Collaborator Author

zyga commented Sep 12, 2019

Dear reviewers, this patch really comprises of a one-line addition and two-column removal https://github.com/snapcore/snapd/pull/7448/files#diff-36e872aea1fc7dba34c7dfa820f13ac0R108

@zyga
Copy link
Collaborator Author

zyga commented Sep 12, 2019

Merging with +1 in a comment from @anonymouse64 above.

@zyga zyga merged commit 3c643e3 into snapcore:master Sep 12, 2019
@zyga zyga deleted the tweak/no-mount-ids-ns-test branch September 12, 2019 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants