-
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
System test fixes for new simplified read-pool #277
Conversation
Manual system tests [failure] with the following config
|
Manual system tests [failure] with the following config
|
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.
some minor comments/queries
"tokens": 0.4, | ||
"duration": "1h", | ||
}), true) | ||
output, err = readPoolLock(t, configPath, "--tokens 0.4", true) |
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 createParams logic above is consistent with the rest of the test codebase, why has it been removed in favour of literal strings?
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.
according to my testing, boolean flag doesn't work with parameter map, and read pool lock command has boolean flag. hence all the call to lock read-pool has been changed to look similar in case we need to use the parameter more in the future.
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.
you can pass through a boolean with
output, err = readPoolLock(t, configPath, createParams(map[string]interface{}{
"booleanName": "",
}), true)
or
output, err = readPoolLock(t, configPath, createParams(map[string]interface{}{
"booleanName": "true",
}), true)
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.
see how we do it for the commit flag for example
https://github.com/0chain/system_test/pull/282/files#diff-b64b1f0b1e26c3819b06b4df6e3961b113082b9e744f7b4ea499371cf8459c89R60
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 would work with bool values that assume the default value to be false. I have added a fix in createParams that would work with boolean values directly, so we don't have to use blank string for false values.
Also reversed all the strings back to params using createParams.
PTAL.
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.
Thanks, much tidier
Manual system tests [success] with the following config
|
No description provided.