HPCC4J-561 copyfile test wait for spray completion #667
Conversation
|
@rpastrana please review |
| } | ||
| } while (waitCount < 60 && dfuProgress.getPercentDone() < 100); | ||
|
|
||
| assumeTrue("File spray never completed", waitCount < MAX_WAIT_COUNT); |
There was a problem hiding this comment.
Minor, perhaps picky, but the message might be a tiny bit misleading, we don't know if the file spray completed, but we do know the test timed out
| Thread.sleep(1000); | ||
| } | ||
| Thread.sleep(1000); | ||
| System.out.println("Percent complete: " + dfuProgress.getPercentDone() + " Sleeping for 1sec to wait for spray."); |
There was a problem hiding this comment.
Minor because we're not likely to see this message often, but when we do, it feels incomplete because we didn't announce we're spraying a file and I'm not sure it'll be obvious why we're reporting status for a spray.
| System.out.println("Still spraying, waiting 1 sec..."); | ||
| for (int i=wait;i>0;i--) | ||
| dfuProgress = wsclient.getFileSprayClient().getDfuProgress(dfuspray.getWuid()); | ||
| if (!whiteListedStates.contains(dfuProgress.getState())) { |
There was a problem hiding this comment.
Since we're testing for a failure here, it feels more natural to me to seek for one of any blacklisted states instead. Is there something odd about the failure states that swayed you to seek for absence of whitelisted states?
There was a problem hiding this comment.
I generally go for whitelists because they are explicit and more likely to catch errors / future errors. IE: If a new state gets added the whitelist will treat it as an error but the blacklist would ignore it.
There was a problem hiding this comment.
Fair enough, but the original enum on the platform side doesn't seem to have changed in a while:
https://github.com/hpcc-systems/HPCC-Platform/blame/7552030a5bb03b63d4af0d37d454f8eab22feb9b/dali/dfu/dfuwu.hpp#L40
If that is the right enum, at this point I would consider it static.
With that in mind, it seems seeking explicitly for one of the known failed states will lead to less false positives.
- Modified WSFileIOClientTest.copyfile test to correctly monitor spray progress - Updated remote unit tests values.yaml to keep file spray service Signed-off-by: James McMullan [email protected]
|
@rpastrana Pushed up code review changes & squashed |
Signed-off-by: James McMullan [email protected]
Type of change:
Checklist:
Testing: