Require a configuration backup to be readable by its own restore - #733
Merged
Conversation
The two halves of this class were described twice and never compared: backup() was asserted against "some string was saved", and restore() against a blob the test packed for itself. A change to how the configuration is packed — a different compression, an added encoding step — would leave both green while every backup already stored became unreadable, and nobody would find out until a restore was needed. So back a configuration up, feed back exactly what was stored, and require the same configuration out the other side, with values that are not defaults so the comparison means something. Swapping gzcompress for gzdeflate in the packing makes it fail, which is the kind of change it exists to catch.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
One test, for a property the class had two tests about and never actually asserted.
The gap
ConfigBackup's halves were described twice, separately:testBackupasserted thatsave()was called with a string;testRestoreasserted that a blob the test packed itself —bin2hex(gzcompress(serialize(…)))— could be restored.Neither says the two halves agree. A change to how the configuration is packed (a different compression, an added encoding step) would leave both green while every backup already stored became unreadable — discovered on the day somebody needed a restore.
The test
Back a configuration up, feed back exactly what was stored, and require the same configuration out the other side. The values are deliberately not defaults — a database name, a session timeout, a theme, a mail flag, an allowed-mime list — so the comparison means something rather than passing on two empty objects.
Verified to fail for the reason it exists: swapping
gzcompressforgzdeflateinpackConfigData()breaks it.One thing worth knowing for the next test here
The capture uses a closure with
use (&$restored), not an arrow function: an arrow function captures by value at creation, and at that point nothing has been restored yet. It cost me a round to find, so it is written down in the test.Unit suite green: 3093.