-
Notifications
You must be signed in to change notification settings - Fork 82
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
add random operation test #1575
base: master
Are you sure you want to change the base?
Conversation
9a63bad
to
aaceaf5
Compare
set_param, | ||
set_param, |
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.
set_param, | |
set_param, | |
set_param, |
if step_name: | ||
step_name_list.append(step_name) | ||
except CmdException: | ||
step_name_list.append(step_name) |
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.
step.__name__ ?
used_core_devices = devices.get("used_core_devices") | ||
|
||
if not available_cache_devices: | ||
return None |
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.
make returns consistent (either just return
or return None
)
devices = { | ||
"used_cache_devices": used_cache_devices, | ||
"available_cache_devices": available_cache_devices, | ||
"used_core_devices": used_core_devices, | ||
"available_core_devices": available_core_devices, | ||
} |
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.
I really don't really like the idea of putting the lists in a dict and in each operation function getting them back using the string keys.
Alternatives:
- make the lists global
- define the operation functions within the test so they can access those lists directly
- make a class with the lists as variables
Preferably one of the first two.
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.
Overall looks good - minor suggestions
Signed-off-by: Kamil Gierszewski <[email protected]>
aaceaf5
to
acff040
Compare
No description provided.