Added negative tests for restore of snapshots #173
Merged
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
ashwani2k
requested review from
georgekuruvillak,
shreyas-s-rao and
swapnilgm
as code owners
|
|
swapnilgm
added
area/quality
amshuman-kr
suggested changes
Jul 16, 2019
| } | ||
| logger.Infoln("starting restore while snapshotter is running") | ||
| err = rstr.Restore(RestoreOptions) | ||
| Expect(err).Should(HaveOccurred()) |
There was a problem hiding this comment.
Should we add a check that the existing etcd data is left untouched?
| return fmt.Errorf("entry not found for key %s", resKey) | ||
| //handles deleted keys as every 10th key is deleted during populate etcd call | ||
| if math.Mod(float64(currKey), 10) == 0 { | ||
| continue //it should continue as key was put for action delete |
There was a problem hiding this comment.
Should we check that it is indeed deleted/not there?
shreyas-s-rao
requested changes
Jul 24, 2019
There was a problem hiding this comment.
@ashwani2k Thanks for the well-written and much-needed PR to improve test coverage. I have a few suggestions. Please make the necessary changes:
- Negative test separation: Ginkgo allows to run or skip specific specs or spec containers (Describe, Context, etc). You can change the text passed to the second Describe function to something like
NEGATIVE: For Dynamic Loads and Negative Scenarios, and then pass the flag--focus="NEGATIVE\:.*"to run only negative tests, or--skipto skip them. This way, we can omit the--randomizeAllSpecsflag while running the negative tests in a separate command within the unit test script. As the negative tests for restoration don't use the pre-populated snapstore, there will not be a time-overhead as well. This also makes it easy to debug when tests fail, as we'll know whether negative or non-negative tests failed.
ashwani2k
force-pushed
the
improve-restore-tests
branch
3 times, most recently
from
dc5cb5d to
9901518
Compare
ashwani2k
force-pushed
the
improve-restore-tests
branch
from
9901518 to
7758833
Compare
ashwani2k
force-pushed
the
improve-restore-tests
branch
from
7758833 to
caf9c6b
Compare
shreyas-s-rao
approved these changes
Sep 4, 2019
There was a problem hiding this comment.
@ashwani2k Thanks for addressing all the suggestions! LGTM 👍🏼
shreyas-s-rao
added
reviewed/ok-to-test
gardener-robot-ci-1
added
needs/ok-to-test
gardener-robot
added
priority/3
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.
What this PR does / why we need it:
Added more negative tests for restore of snapshots.
Following test been added -
Which issue(s) this PR fixes:
Fixes partially #73 #104
Special notes for your reviewer:
Enhanced the populate script to capture delete of keys.
Some refactoring is done to segregate the tests into 2 categories -
Release note: