-
Notifications
You must be signed in to change notification settings - Fork 16
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
Hotfix/initialization checks #444
Conversation
jeremyjams
commented
Oct 18, 2021
- Added checks before upgrading locally to initialized status
- Removed 2-blocks waiting time before supplying new replicate
- Minor logs update
assertThat(task.getDateStatusList().get(task.getDateStatusList().size() - 1).getStatus()).isEqualTo(INITIALIZING); | ||
assertThat(task.getCurrentStatus()).isEqualTo(INITIALIZING); |
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.
Aren't those 2 lines redundant? It looks like we are sure that the last status in the dateStatusList
is equal to the current status.
(That's enforced in Task
class so I'm not sure we should test it here.)
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 see your point yes, I might say current version helps reading. Let's take this example:
assertThat(task.getDateStatusList().get(task.getDateStatusList().size() - 3).getStatus()).isEqualTo(RECEIVED);
assertThat(task.getDateStatusList().get(task.getDateStatusList().size() - 2).getStatus()).isEqualTo(INITIALIZING);
assertThat(task.getDateStatusList().get(task.getDateStatusList().size() - 1).getStatus()).isEqualTo(INITIALIZED);
assertThat(task.getCurrentStatus()).isEqualTo(INITIALIZED);
- Last assertion helps reading quickly what's the targeted status at the end.
- Three first assertions are kind of optional tests checking status orders, easy to read them out in one block.
Anyway, if we can discuss on that, I don't feel confident with refactoring these tests in this hotfix PR.
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.
Alright, I'm OK with that. Thanks!
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.
Good, thanks!
Ok for me, thanks! |