-
Notifications
You must be signed in to change notification settings - Fork 7
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
Golden test suite for snapshot codec #499
base: main
Are you sure you want to change the base?
Conversation
So a big question I have is should this be a stand alone test suite (as it currently is) or should I graft it into the "monolithic" LST Tree test suite. I think the later might be more appropriate, and would appreciate a pointer as to where a good place in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new Bounded
and Enum
instances should arguably be orphan instances in the test suite. They are not necessary for the core library to function, or necessary for the user to have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. There are also some comments we discussed over other channels, but I won't repeat them here
8b0d38e
to
4c00cf4
Compare
…e for serialization backwards compatibility testing.
4c00cf4
to
00e7a07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost LGTM! I do have some minor suggestions, but after that we can probably merge
@@ -70,7 +70,7 @@ instance RefCounted m (MergingRun m h) where | |||
getRefCounter = mergeRefCounter | |||
|
|||
data MergePolicyForLevel = LevelTiering | LevelLevelling | |||
deriving stock (Show, Eq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have missed this suggestion: #499 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, did we end up using these instances for the golden tests? It doesn't look like it -- if that's the case, let's remove these instances
@@ -47,6 +49,7 @@ tests = testGroup "Test.Database.LSMTree.Internal.Snapshot.Codec" [ | |||
testAll $ \(p :: Proxy a) -> | |||
testGroup (show $ typeRep p) $ | |||
prop_arbitraryAndShrinkPreserveInvariant @a deepseqInvariant | |||
, goldenFileTests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typical approach we use in this test suite is to include the tests at the top level, so in the Main
module
-- | | ||
-- Compare the serialization of snapshot metadata with a known reference file. | ||
goldenFileTests :: TestTree | ||
goldenFileTests = handleOutputFiles . testGroup "Golden File Comparisons" $ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goldenFileTests = handleOutputFiles . testGroup "Golden File Comparisons" $ | |
goldenFileTests = handleOutputFiles . testGroup "Test.Database.LSMTree.Internal.Snapshot.Codec.Golden" $ |
-- | | ||
-- Compare the serialization of snapshot metadata with a known reference file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the newline intentional? Does haddock render it correctly?
import System.FS.IO (HandleIO, ioHasFS) | ||
import qualified Test.Tasty as Tasty | ||
import Test.Tasty (TestName, TestTree, testGroup) | ||
import qualified Test.Tasty.Golden as Au |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Au
as in the chemical symbol for gold? 😄
testCodecBuilder tName metadata = | ||
testGroup tName $ uncurry snapshotCodecTest <$> metadata | ||
|
||
type ComponentAnnotation = String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not immediately clear to me what String
is used for. Maybe add some documentation?
enumerateSnapshotLabel :: [(ComponentAnnotation, SnapshotLabel)] | ||
enumerateSnapshotLabel = | ||
[ ("Bs", SnapshotLabel "UserProvidedLabel") | ||
, ("Bn", SnapshotLabel "") | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think I understand now that the ComponentAnnotation
is used to name the specific enumerations, and this is reflected in the names of the golden files. Can you explain how you picked these labels? And the labels for the other enumerators below?
Description
Add the golden test cases to ensure that metadata serialization does not accidentally change. This will help ensure we maintain backwards compatibility. Changes in backwards compatibility should case test case failures.