tests: remove is_hdd #9712
tests: remove is_hdd #9712
Conversation
src/common/util.cpp
Outdated
| // If the device returned by stat does not exist at /sys/dev/block/, we are likely | ||
| // inside a container. There is no way to check if file_path is on a hard drive. | ||
| MWARNING("Unable to determine whether file path '" << file_path << "' is on a HDD, assuming not"); | ||
| return false; |
There was a problem hiding this comment.
Shouldn't this return boost::none? Otherwise I can't see a coherent description of the overall function's behavior.
There was a problem hiding this comment.
It returns false to signify that it is not an error (and warns the user instead). Returning boost::none would cause this test case to fail.
There was a problem hiding this comment.
Writing code to make the tests pass, rather than produce coherent function behavior, doesn't make sense. If "cannot determine whether it's an HDD or not" isn't an "unknown", what is it? Please write a coherent description of this function's behavior with your change ("returns false if not on an HDD or can't tell if on an HDD" doesn't work, because "can't tell if on an HDD" applies to all of the none cases prior to this change).
There was a problem hiding this comment.
There is a difference between knowing we can't obtain a value ("unable to determine") and something unexpected happening (like /sys/dev/<device>/queue/rotational not existing or stat returning an error value).
I don't know how to compress 4 possible states: {we know it's an HDD | we know it's not an HDD | we know that we don't know if it's an HDD | something unexpected happened} into 3 values, other than what I've come up with here.
I can try to clarify function behavior, but I don't think we should complicate it further since all it's used for is showing a warning IFF we know the blockchain is stored on a HDD.
There was a problem hiding this comment.
I don't know how to compress 4 possible states: {we know it's an HDD | we know it's not an HDD | we know that we don't know if it's an HDD | something unexpected happened} into 3 values, other than what I've come up with here.
This is precisely the misapprehension, in my view: There aren't four states, only three. Nearly all of the existing "none" cases occur when a file should be there and openable, but isn't. In those cases, "we know that we don't know" and we also know that "something unexpected happened"; they are the same thing. The same conditions apply in this new case because the block device can't be queried, at all; prior to this change, that would have meant returning none, implying you now have a backwards-incompatible behavior, yet no coherent description of the new behavior.
If you want to do what the PR title says, then do it: Avoid showing a warning in the unknown case. Or change the message. If you wanted to alter the return type of the function to accommodate a new state, you certainly could, but that would require an actual description of the state, which still does not exist.
There was a problem hiding this comment.
Which do you prefer: remove the warning and return false, or rewrite the function to return a bool and remove the tests?
There was a problem hiding this comment.
Which do you prefer: remove the warning and return false or rewrite the function to return a bool and remove the tests?
Neither. Make no changes, as I see no good reason for them.
There was a problem hiding this comment.
Ok. What would you suggest as an alternative approach to get tests to pass when run inside a container? Exclude this specific test? A more general solution would have my preference.
There was a problem hiding this comment.
Is your goal here just fixing something in the test suite? If so, I agree, it's broken, just remove it.
There was a problem hiding this comment.
The logic of the test is broken: On "what appears to be like linux", this shouldn't return none. Well, except, it obviously can, and sometimes should. If you want to keep the no __GLIBC__ test, I guess you can, but it's also rather contrived.
tobtoht
force-pushed
the
common_is_hdd_container
branch
from
1ec65be to
a09020b
Compare
tobtoht
changed the title
Does not work inside containers. Has other issues: see discussion.
Required for #9708.