----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63245/#review189074 -----------------------------------------------------------
This review request should really be at least three separate patches. THere is work to be done to fix several of these tests, and right now they're all mixed together. 3rdparty/stout/include/stout/os.hpp Line 120 (original), 120 (patched) <https://reviews.apache.org/r/63245/#comment265988> We can't do this, it's nonsense. `PATH` does not make sense as an environment variable here, as, like you said, there is no equivalent to `LD_LIBRARY_PATH` on Windows. 3rdparty/stout/include/stout/os/windows/stat.hpp Line 112 (original), 112 (patched) <https://reviews.apache.org/r/63245/#comment265989> Why was this change made? 3rdparty/stout/include/stout/windows/os.hpp Lines 390-392 (patched) <https://reviews.apache.org/r/63245/#comment265990> This should be an `Error`, not a `WindowsError`, as the latter is reserved for OS errors (explicitly, its implementation calls `GetLastError()` and retrieves the Windows error message, none of which should happen here). While the POSIX implementation does not explicitly check for `duration < 0`, since the system call itself fails, and that failure is returned, I agree that it's reasonable here to return an `Error`. However, a note should be added to the function as to why we do that (specificically, so that the `os::sleep` API remains consistent). 3rdparty/stout/tests/os_tests.cpp Lines 213-215 (original), 213-215 (patched) <https://reviews.apache.org/r/63245/#comment265991> If this is resolved, this comment needs to be removed. 3rdparty/stout/tests/os_tests.cpp Lines 232-233 (original) <https://reviews.apache.org/r/63245/#comment265992> Removing this test code does not resolve this issue. In fact, it's reducing test coverage, as there is clearly a bug here highlighted by this failing test. Instead, I agree with you here: > If you are supposed to be able to test the size of a non-link file in the context of not-following-a-link, the program should return the size of the non-link file instead of returning an error upon determining the nature of the path as not being a link file path. So the correct solution is to fix `os::stat::size` on Windows to work correctly, at which point this test can be enabled. 3rdparty/stout/tests/os_tests.cpp Lines 274-275 (original), 272-273 (patched) <https://reviews.apache.org/r/63245/#comment265993> If this is working now, the TODO here should be removed. 3rdparty/stout/tests/os_tests.cpp Lines 885-925 (original), 883-923 (patched) <https://reviews.apache.org/r/63245/#comment265994> Having changed `ldLibraryPath` above to `PATH`, this just potentially broke numerous things by overriding the system's `PATH` (hopefully just for this process, but I digress, it's incorrect). This test sh ould probably _never_ be enabled on Windows, as there is simply no equivalent to `LD_LIBRARY_PATH`. Windows loads libraries from a static set of paths that cannot be changed. Instead of enabling this test, it should be explicitly `#ifdef'd` out with an explanation for Windows that the API simply cannot be ported. 3rdparty/stout/tests/os_tests.cpp Line 1023 (original), 1021 (patched) <https://reviews.apache.org/r/63245/#comment265995> This is incorrect. This test is explicitly asserting that the `testLink` resolved to the `testFile`. Changing the test is wrong. Instead, there is a set of tasks to fix `realpath` on Windows, including MESOS-6735, MESOS-5881, and even MESOS-7604. For now, we'll leave fixing `realpath` separate to fixing these other tests. - Andrew Schwartzmeyer On Oct. 24, 2017, 2:30 a.m., Raluca Miclea wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63245/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2017, 2:30 a.m.) > > > Review request for mesos and Andrew Schwartzmeyer. > > > Bugs: MESOS-3441 > https://issues.apache.org/jira/browse/MESOS-3441 > > > Repository: mesos > > > Description > ------- > > TEST_F(OsTest, Libraries) > The test failed because no environmental variable was provided for > Windows. I used "PATH" as environmental variable on Windows since > there's no default variable where the linker should look into while > linking dynamic libraries/shared libraries. (like LD_LIBRARY_PATH on > Unix/Linux) > > TEST_F(OsTest, Sleep) > The test hanged because of the last assertion: > ASSERT_ERROR(os::sleep(Milliseconds(-10))); > Apparently, it didn't handle a negative sleep duration. By simply > returning an error value when the duration is negative the problem was > solved. > > TEST_F(OsTest, SYMLINK_Size) > First issue was this assertion: > EXPECT_SOME_EQ(size, > os::stat::size(file,FollowSymlink::DO_NOT_FOLLOW_SYMLINK)); > It fails everytime because <file> is not a symbolic link. When calling > os:stat:size with argument <follow> set to "DO_NOT_FOLLOW_SYMLINK", it firstly > checks whether the path it receives is a symlink in order to get the symlink > data > for that path. If it's not a symlink, it returns an error. > If you are supposed to be able to test the size of a non-link file in > the context of not-following-a-link, the program should return the > size of the non-link file instead of returning an error upon determining the > nature of the path as not being a link file path. Right now, the assertion > is not relevant since you'll get an error if you supply it a non-link file > path rather thana symlink. > > TEST_F(OsTest, SYMLINK_Realpath) > The issue was that the symlink resolved to itself but it was tested > against the path of the target file. Once the path of the target file > was replaced by the path of the symlink, the test passed. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os.hpp > 1a81db61faa55d7043d75a012fe1e66b49bf601c > 3rdparty/stout/include/stout/os/windows/stat.hpp > 0db6d482ad19897db33d334bffe4b294da11a36f > 3rdparty/stout/include/stout/windows/os.hpp > 09ddec6d69472cd13b453fe1a77fdbe343fc23c8 > 3rdparty/stout/tests/os_tests.cpp 959b02d08f06481a64ef1a39acf23b062d5dc805 > > > Diff: https://reviews.apache.org/r/63245/diff/1/ > > > Testing > ------- > > Modified tests: > TEST_F(OsTest, Libraries) > TEST_F(OsTest, Sleep) > TEST_F(OsTest, SYMLINK_Size) > TEST_F(OsTest, SYMLINK_Realpath) > > > Thanks, > > Raluca Miclea > >
