-
Notifications
You must be signed in to change notification settings - Fork 5
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
Modify/datetime #330
Modify/datetime #330
Conversation
Manual system tests [failure] with the following config
|
Manual system tests [success] with the following config
|
@@ -109,7 +109,8 @@ func TestCreateDir(t *testing.T) { | |||
output, err := createDir(t, configPath, allocID, longDirName, false) | |||
require.NotNil(t, err, "expected create dir failure command executed with output: ", strings.Join(output, "\n")) | |||
require.Len(t, output, 1) | |||
require.True(t, strings.HasPrefix(output[0], `CreateDir failed: {"error":"ERROR: value too long for type character varying(100) (SQLSTATE 22001)"}`), "expected create dir failure command executed with output: ", strings.Join(output, "\n")) | |||
aggregatedOutput := strings.Join(output, " ") | |||
require.Contains(t, aggregatedOutput, "consensus failed") |
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.
This seems like a backward change. The error should be something meaningful, atleast saying createDir failed.
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.
Actually I thought about it. But you know, when blobbers are rejecting commit request, it means consensus is not met.
If an allocation would have 128 blobbers, then there will be 128 messages. Gosdk can't figure out which error to consider for.
Maybe we could collect common messages among blobbers and return to the user with large count.
Previous directory creation process was incomplete.
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.
"when blobbers are rejecting commit request, it means consensus is not met."
based on this, let's translate the error message to "directory creation failed" - something a bit more meaningful to the end user
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.
agree with Kishan - a better error message is needed, even if it is just create failed
@@ -349,7 +322,8 @@ func TestCreateDir(t *testing.T) { | |||
output, err = createDirForWallet(t, configPath, nonAllocOwnerWallet, true, allocID, true, "/mydir", false) | |||
require.NotNil(t, err, "Expected create dir failure but got output: ", strings.Join(output, "\n")) | |||
require.Len(t, output, 1) | |||
require.True(t, strings.HasPrefix(output[0], `CreateDir failed: {"code":"invalid_signature","error":"invalid_signature: Invalid signature"}`), "Expected create dir failure but got output: "+strings.Join(output, "\n")) | |||
aggregatedOutput := strings.Join(output, " ") | |||
require.Contains(t, aggregatedOutput, `consensus not met`) |
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.
Same as above
Modify system tests w.r.t. changes in PR 0chain/blobber#748.