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 58673: Fix FlagsFileTest to check for absolute path properly on Windows.
Date Thu, 27 Apr 2017 00:23:41 GMT


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 79-86 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699763#file1699763line79>
> >
> >     I think this got a bit out of date. We settled on `\...` being an absolute path,
not just `\?...` (per below implementation).
> 
> Jeff Coffler wrote:
>     I didn't have a test for this specifically. The above is accurate, though. The 1-3
cases are for files on disk. Network shares are a special case and, while technically an absolute
path, isn't related to files on disk.
>     
>     That said, the function does implement this properly. The comment isn't out of date.
The note was just to point out that we handled this as well.

Sorry, I mean that your third bullet should read `3. "\..."` instead of `3. "\?..."`.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58673/#review173000
-----------------------------------------------------------


On April 25, 2017, 6:24 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58673/
> -----------------------------------------------------------
> 
> (Updated April 25, 2017, 6:24 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-5937
>     https://issues.apache.org/jira/browse/MESOS-5937
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform,
> but disabled tests that did not pass. Enabled absolute path tests,
> adding tests that were appropriate for the Windows platform.
> 
> Note that, for Windows, absolute paths may not be valid. For example,
> a path like "\\?\abc:file.txt" is not valid. In this case, the
> path::absolute method is undefined; the expectation is that it is
> called with valid paths (absolute or relative, but valid).
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/flags/parse.hpp 65edd86372596c2107e9f29cf27301e025e6620e

>   3rdparty/stout/include/stout/path.hpp 2d2088aadfa1ea82c59424242671c4fb655dede1 
>   3rdparty/stout/tests/CMakeLists.txt 4bbe713f259e7858d423dcb33956d41e62a915eb 
>   3rdparty/stout/tests/flags_tests.cpp e2681f8f68f6478d4c8a20c1e75ddb050d16b56d 
>   3rdparty/stout/tests/path_tests.cpp 0490d93908566c46a10d91b05790e5a7f2f289bc 
> 
> 
> Diff: https://reviews.apache.org/r/58673/diff/2/
> 
> 
> Testing
> -------
> 
> Passes `make check` on the Linux platform.
> 
> On Windows, the FlagsFileTest.JSONFile now passes, along with new PathTest.Absolute tests
that I added due to new path::absolute handling on the Windows platform.
> 
> PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*FlagsFileTest*"
> Note: Google Test filter = *FlagsFileTest*-
> [==========] Running 2 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 2 tests from FlagsFileTest
> [ RUN      ] FlagsFileTest.JSONFile
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W0425 10:05:20.959357 1060440 parse.hpp:97] Specifying an absolute filename to read a
command line option out of without using 'file:// is deprecated and will be removed in a future
release. Simply adding 'file://' to the beginning of the path should eliminate this warning.
> [       OK ] FlagsFileTest.JSONFile (9 ms)
> [ RUN      ] FlagsFileTest.FilePrefix
> [       OK ] FlagsFileTest.FilePrefix (6 ms)
> [----------] 2 tests from FlagsFileTest (18 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 2 tests from 1 test case ran. (23 ms total)
> [  PASSED  ] 2 tests.
> PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe --gtest_filter="*PathTest*"
> Note: Google Test filter = *PathTest*-
> [==========] Running 2 tests from 1 test case.
> [----------] Global test environment set-up.
> [----------] 2 tests from PathTest
> [ RUN      ] PathTest.Absolute
> [       OK ] PathTest.Absolute (1 ms)
> [ RUN      ] PathTest.Comparison
> [       OK ] PathTest.Comparison (0 ms)
> [----------] 2 tests from PathTest (2 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 2 tests from 1 test case ran. (6 ms total)
> [  PASSED  ] 2 tests.
> 
>   YOU HAVE 4 DISABLED TESTS
> 
> PS C:\mesos>
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>


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