From a47fae066b6fba3b6af074368e30ffec4cef65aa Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Thu, 28 Nov 2024 15:05:34 +0100 Subject: [PATCH 1/3] disk: cover all entities in PartitionTable.features() 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 --- pkg/disk/partition_table.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/disk/partition_table.go b/pkg/disk/partition_table.go index 04c64ecda7..1fc4199afb 100644 --- a/pkg/disk/partition_table.go +++ b/pkg/disk/partition_table.go @@ -841,16 +841,20 @@ type partitionTableFeatures struct { Swap bool } -// features examines all of the PartitionTable entities -// and returns a struct with flags set for each feature used +// features examines all of the PartitionTable entities and returns a struct +// with flags set for each feature used. The meaning of "feature" here is quite +// broad. Most disk Entity types are represented by a feature and the existence +// of at least one type in the partition table means the feature is +// represented. For Filesystem entities, there is a separate feature for each +// filesystem type func (pt *PartitionTable) features() partitionTableFeatures { var ptFeatures partitionTableFeatures introspectPT := func(e Entity, path []Entity) error { switch ent := e.(type) { - case *LVMLogicalVolume: + case *LVMVolumeGroup, *LVMLogicalVolume: ptFeatures.LVM = true - case *Btrfs: + case *Btrfs, *BtrfsSubvolume: ptFeatures.Btrfs = true case *Filesystem: switch ent.GetFSType() { @@ -867,6 +871,10 @@ func (pt *PartitionTable) features() partitionTableFeatures { ptFeatures.Swap = true case *LUKSContainer: ptFeatures.LUKS = true + case *PartitionTable, *Partition: + // nothing to do + default: + panic(fmt.Errorf("unknown entity type %T", e)) } return nil } From 5974c14a6e3f47621320c61a839dc7641d6cc472 Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Fri, 29 Nov 2024 13:01:05 +0100 Subject: [PATCH 2/3] disk,osbuild: use t.Name() in test update instruction message 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. --- pkg/disk/disk_test.go | 9 +++++---- pkg/osbuild/fstab_stage_test.go | 8 +++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/disk/disk_test.go b/pkg/disk/disk_test.go index 372b073c0c..15b40ece46 100644 --- a/pkg/disk/disk_test.go +++ b/pkg/disk/disk_test.go @@ -987,22 +987,23 @@ func TestForEachFSTabEntity(t *testing.T) { } for name := range testdisk.TestPartitionTables { - t.Run(name, func(t *testing.T) { - + // use a different name for the internal testing argument so we can + // refer to the global test by t.Name() in the error message + t.Run(name, func(ts *testing.T) { var targets []string targetCollectorCB := func(ent disk.FSTabEntity, _ []disk.Entity) error { targets = append(targets, ent.GetFSFile()) return nil } - require := require.New(t) + require := require.New(ts) pt := testdisk.TestPartitionTables[name] // print an informative failure message if a new test partition // table is added and this test is not updated (instead of failing // at the final Equal() check) exp, ok := expectedEntityPaths[name] - require.True(ok, "expected options not defined for test partition table %q: please update the TestNewFSTabStageOptions test", name) + require.True(ok, "expected test result not defined for test partition table %q: please update the %s test", name, t.Name()) err := pt.ForEachFSTabEntity(targetCollectorCB) // the callback never returns an error, but let's check it anyway diff --git a/pkg/osbuild/fstab_stage_test.go b/pkg/osbuild/fstab_stage_test.go index cd12f28246..6797980bf0 100644 --- a/pkg/osbuild/fstab_stage_test.go +++ b/pkg/osbuild/fstab_stage_test.go @@ -104,8 +104,10 @@ func TestNewFSTabStageOptions(t *testing.T) { } // Use the test partition tables from the disk package. for name := range testdisk.TestPartitionTables { - t.Run(name, func(t *testing.T) { - require := require.New(t) + // use a different name for the internal testing argument so we can + // refer to the global test by t.Name() in the error message + t.Run(name, func(ts *testing.T) { + require := require.New(ts) pt := testdisk.TestPartitionTables[name] // math/rand is good enough in this case @@ -118,7 +120,7 @@ func TestNewFSTabStageOptions(t *testing.T) { // table is added and this test is not updated (instead of failing // at the final Equal() check) exp, ok := expectedOptions[name] - require.True(ok, "expected options not defined for test partition table %q: please update the TestNewFSTabStageOptions test", name) + require.True(ok, "expected test result not defined for test partition table %q: please update the %s test", name, t.Name()) options, err := NewFSTabStageOptions(&pt) require.NoError(err) From db5fec022e448d6723420f6e46b1b1d0e05e179d Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Fri, 29 Nov 2024 13:07:52 +0100 Subject: [PATCH 3/3] disk: update TestPartitionTableFeatures() to catch missing test case 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. --- pkg/disk/partition_table_test.go | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/disk/partition_table_test.go b/pkg/disk/partition_table_test.go index 2924a5f00b..7377bf5cac 100644 --- a/pkg/disk/partition_table_test.go +++ b/pkg/disk/partition_table_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/osbuild/images/internal/testdisk" "github.com/osbuild/images/pkg/blueprint" @@ -2284,21 +2285,24 @@ func TestNewCustomPartitionTableErrors(t *testing.T) { } func TestPartitionTableFeatures(t *testing.T) { - type testCase struct { - partitionType string - expectedFeatures disk.PartitionTableFeatures - } - testCases := []testCase{ - {"plain", disk.PartitionTableFeatures{XFS: true, FAT: true}}, - {"plain-swap", disk.PartitionTableFeatures{XFS: true, FAT: true, Swap: 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}}, - } + require := require.New(t) - for _, tc := range testCases { - pt := testdisk.TestPartitionTables[tc.partitionType] - assert.Equal(t, tc.expectedFeatures, disk.GetPartitionTableFeatures(pt)) + testCases := map[string]disk.PartitionTableFeatures{ + "plain": {XFS: true, FAT: true}, + "plain-noboot": {XFS: true, FAT: true}, + "plain-swap": {XFS: true, FAT: true, Swap: true}, + "luks": {XFS: true, FAT: true, LUKS: true}, + "luks+lvm": {XFS: true, FAT: true, LUKS: true, LVM: true}, + "btrfs": {XFS: true, FAT: true, Btrfs: true}, + } + for name := range testdisk.TestPartitionTables { + // print an informative failure message if a new test partition + // table is added and this test is not updated (instead of failing + // at the final Equal() check) + exp, ok := testCases[name] + require.True(ok, "expected test result not defined for test partition table %q: please update the %s test", name, t.Name()) + pt := testdisk.TestPartitionTables[name] + require.Equal(exp, disk.GetPartitionTableFeatures(pt)) } }