mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 63245: Ported and enabled Os Tests on Windows.
Date Tue, 24 Oct 2017 18:55:08 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message