disk: cover all entities in PartitionTable.features() #1074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achilleas-k
force-pushed
the
disk/all-the-features
branch
from
2c84447
to
c755ed6
Compare
mvo5
previously approved these changes
Nov 29, 2024
There was a problem hiding this comment.
I like it, tiny suggestion inline.
Fwiw, I was debating in my head if we should panic() or error, here it seems panic is okay, I was wondering if we could make it easier to ensure that we catch when new types are added, maybe something like this:
diff --git a/pkg/disk/partition_table_test.go b/pkg/disk/partition_table_test.go
index 2924a5f00..258648e72 100644
--- a/pkg/disk/partition_table_test.go
+++ b/pkg/disk/partition_table_test.go
@@ -2291,10 +2291,13 @@ func TestPartitionTableFeatures(t *testing.T) {
testCases := []testCase{
{"plain", disk.PartitionTableFeatures{XFS: true, FAT: true}},
{"plain-swap", disk.PartitionTableFeatures{XFS: true, FAT: true, Swap: true}},
+ {"plain-noboot", disk.PartitionTableFeatures{XFS: true, FAT: true}},
{"luks", disk.PartitionTableFeatures{XFS: true, FAT: true, LUKS: true}},
{"luks+lvm", disk.PartitionTableFeatures{XFS: true, FAT: true, LUKS: true, LVM: true}},
{"btrfs", disk.PartitionTableFeatures{XFS: true, FAT: true, Btrfs: true}},
}
+ assert.Equal(t, len(testCases), len(testdisk.TestPartitionTables),
+ "new partition table test cases got added but features not updated")
for _, tc := range testCases {
pt := testdisk.TestPartitionTables[tc.partitionType]would ensure this - but I'm also probably overthinking it as it will be a bit before we add new Entities I suppose (but who knows what we need if bcachefs becomes a thing ;)
Add a case for every entity type in the PartitionTable.features() callback. More importantly, panic if any entity type is not covered by a case. This ensures that when a new entity type is added, we need to consider whether it needs to be represented as a feature, added to an existing one, or ignored. Co-authored-by: Michael Vogt <[email protected]>
Use t.Name() in the tests that use the TestPartitionTables and expect all cases to be covered so that we don't need to keep the message in sync with the function name. The message for TestForEachFSTabEntity() was wrong.
Update the TestPartitionTableFeatures() to match other tests that use the TestPartitionTables. - Switch to require from assert to stop test execution if something fails. This makes it easier to see the important error message. With assert, execution would continue when `ok` is false, which would make the assert.Equal() test print an error too (exp would be an empty initialised struct of PartitionTableFeatures) and can make it hard to notice that the "expected test result not defined" error was printed first. - Use a map for testCases so we can get each test case by name (the order is not important). - Print the same error message as the other similar tests, using t.Name() to refer to the test function name. - Add plain-noboot test case, which was missing from the test.
achilleas-k
dismissed stale reviews from thozza, mvo5, and ondrejbudai
via
db5fec0
achilleas-k
force-pushed
the
disk/all-the-features
branch
from
c755ed6
to
db5fec0
Compare
I think panicking is appropriate for these kinds of programming level errors.
I did a thing. PTAL. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up from #1072 (#1072 (comment))
Add a case for every entity type in the PartitionTable.features() callback. More importantly, panic if any entity type is not covered by a case. This ensures that when a new entity type is added, we need to consider whether it needs to be represented as a feature, added to an existing one, or ignored.
Co-authored-by: Michael Vogt [email protected]