Return errors in rollback/rebuild commands when using snapshots #2044
Conversation
This commit returns errors in the rollback/rebuild commands if any channel is bootstrapped from snapshot. For more details, see FAB-17800 Signed-off-by: manish <manish.sethi@gmail.com>
manish-sethi
force-pushed
the
error_rollback
branch
from
c6c0392 to
a16b393
Compare
| require.False(t, os.IsNotExist(err)) | ||
| } | ||
|
|
||
| // this should return an error as no ledger has been set up |
There was a problem hiding this comment.
The test name TestRebuildDBsCmd() implies a positive test, but it is actually a negative test to verify an error, should we update the test name to indicate the scenario?
There was a problem hiding this comment.
The unit test at this level is all about whether the code in this package invokes the actual code in the kvledger package. So, with that intention it's still a positive test. Checking for error case rather helps minimizing the dependency on the kvledger package as it does not need to know internal details of kvledger package. Same for the below comments as well.
There was a problem hiding this comment.
Ok, given the good code comments, I agree.
| expectedErr := "ledgerID [ch1] does not exist" | ||
| require.Equal(t, expectedErr, err.Error()) | ||
| // this should return an error as no ledger has been set up | ||
| require.Contains(t, err.Error(), "error while checking if any ledger has been bootstrapped from snapshot") |
There was a problem hiding this comment.
The test name TestRollbackCmd() implies a positive test, but it is actually a negative test to verify an error, should we update the test name to indicate the scenario?
| @@ -21,5 +21,5 @@ func TestUpgradeDBsCmd(t *testing.T) { | |||
| defer os.RemoveAll(testPath) | |||
|
|
|||
| cmd := upgradeDBsCmd() | |||
| require.NoError(t, cmd.Execute()) | |||
| require.EqualError(t, cmd.Execute(), "the data format is already up to date. No upgrade is required") | |||
There was a problem hiding this comment.
The test name TestUpgradeDBsCmd() implies a positive test, but it is actually a negative test to verify an error, should we update the test name to indicate the scenario?
Signed-off-by: manish manish.sethi@gmail.com
Type of change
Description
Additional details
see [FAB-17800](https://jira.hyperledger.org/browse/FAB-17800