Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BF(TST): make tests use _path_ helper for Windows "friendliness" of the tests #6955

Merged
merged 4 commits into from
Aug 20, 2022
Merged

BF(TST): make tests use _path_ helper for Windows "friendliness" of the tests #6955

merged 4 commits into from
Aug 20, 2022

Conversation

yarikoptic
Copy link
Member

b91b494 simply annotated those tests which were failing on windows to be skipped without
considering to fix them. Those tests were failing because hardcoded POSIX path separator (/).
Quick solution to have tests fixed is to use path helper around those POSIX paths.
The underlying tested code AFAIK should be ok - CI is yet to prove or disprove it.

Closes #2564

@yarikoptic
BF(TST): make tests use _path_ helper for Windows "friendliness" of t…
30d2650 
…he tests

b91b494 simply annotated those tests which were failing on windows to be skipped without
considering to fix them. Those tests were failing because hardcoded POSIX path separator (/).
Quick solution to have tests fixed is to use _path_ helper around those POSIX paths.
The underlying tested code AFAIK should be ok - CI is yet to prove or disprove it.

Closes datalad#2564
@yarikoptic yarikoptic added the semver-tests Changes only affect tests, no impact on version label Aug 17, 2022
@yarikoptic yarikoptic mentioned this pull request Aug 17, 2022
Closed
@yarikoptic
Copy link
Member Author

uff. codespell became "smarter" - we need to account for that (separate PR)

@yarikoptic yarikoptic mentioned this pull request Aug 17, 2022
Merged
@yarikoptic
Copy link
Member Author

ha -- current fails look legit -- I will try to update my windows VM license in coming days to troubleshoot:

______________________________ test_p_startswith ______________________________
    def test_p_startswith():
        ok_(path_startswith(_p('/a/b'), _p('/a')))
        ok_(path_startswith(_p('/a/b'), _p('/a/b')))
        ok_(path_startswith(_p('/a/b'), _p('/a/b/')))
        ok_(path_startswith(_p('/a/b/'), _p('/a/b')))
        ok_(path_startswith(_p('/a/b'), _p('/')))
        ok_(path_startswith(_p('/aaa/b/c'), _p('/aaa')))
>       nok_(path_startswith(_p('/aaa/b/c'), _p('/aa')))
..\datalad\tests\test_utils.py:945: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
expr = True, msg = None
    def assert_false(expr, msg=None):
        if msg is None:
>           assert not expr
E           assert not True
..\datalad\tests\utils_pytest.py:113: AssertionError
______________________________ test_p_is_subpath ______________________________
    def test_p_is_subpath():
>       ok_(path_is_subpath(_p('/a/b'), _p('/a')))
..\datalad\tests\test_utils.py:954: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
expr = False, msg = None
    def assert_true(expr, msg=None):
        if msg is None:
>           assert expr
E           assert False
..\datalad\tests\utils_pytest.py:191: AssertionError
============================== warnings summary ===============================
datalad/distributed/tests/test_ria_basics.py::test_initremote_basic_httpsurl

@yarikoptic yarikoptic marked this pull request as draft August 17, 2022 21:46
@adswa
Copy link
Member

adswa commented Aug 18, 2022

I can replicate the failure on my windows machine

adswa
adswa reviewed Aug 18, 2022
View reviewed changes
datalad/tests/test_utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 18, 2022 •
edited
Loading

Codecov Report

Merging #6955 (8e90730) into maint (5f0b28d) will increase coverage by 0.99%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6955      +/-   ##
==========================================
+ Coverage   89.97%   90.96%   +0.99%     
==========================================
  Files         354      354              
  Lines       46259    46303      +44     
==========================================
+ Hits        41622    42120     +498     
+ Misses       4637     4183     -454
Impacted Files Coverage Δ
datalad/tests/test_utils.py 97.53% <100.00%> (+0.01%) ⬆️
datalad/_version.py 45.68% <0.00%> (-0.28%) ⬇️
datalad/distributed/tests/test_drop.py 100.00% <0.00%> (ø)
datalad/tests/test_config.py 99.73% <0.00%> (+<0.01%) ⬆️
datalad/config.py 97.26% <0.00%> (+<0.01%) ⬆️
datalad/distributed/drop.py 97.70% <0.00%> (+0.02%) ⬆️
datalad/distribution/tests/test_create_sibling.py 79.83% <0.00%> (+0.17%) ⬆️
datalad/tests/utils.py 65.10% <0.00%> (+11.03%) ⬆️
datalad/tests/test_tests_utils.py 92.34% <0.00%> (+92.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yarikoptic yarikoptic marked this pull request as ready for review August 18, 2022 17:47
@yarikoptic
Copy link
Member Author

Thank you @adswa !!! It is as green as it can get ATM, so ready for merge ;)

@yarikoptic
BF+RF(TST): mark path_prefix back to skip, use custom _p helper for p…
c39a9de 
…aths

test_path_prefix seems to have some genuine failure needing troubleshooting and
my Windows license expired and cannot update ATM to troubleshoot.

The failures for the other two tests seems to boil down to some special
treatment of root (/) path on windows, so we just would make it more correct and explicit
by not using _path_ which just assumes that paths are "legit" and would include
absolute path properly, with custom _p helper which would prepend C: drive to
absolute paths on windows
yarikoptic
yarikoptic commented Aug 18, 2022
View reviewed changes
datalad/tests/test_utils.py
return pm
return p


def test_path_startswith():
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to fix up accidentally mass renamed _path_ -> _p_ here ;) tests should pass and then I will merge -- no code changes, just got tests to pass on exotic Windows. What could go wrong?

yarikoptic and others added 2 commits August 19, 2022 17:37
@yarikoptic @adswa
BF: f-prefix the f-string
16536aa 
Co-authored-by: Adina Wagner <adina.wagner@t-online.de>
@yarikoptic
BF(TST): use introduced _p instead of _path_ an denable test_path_pre…
8e90730 
…fix testing on windows

_path_ did not work because, as it was discovered after, it is not doing
any magic for "absolute posix paths" like /a/c to turn them into legit
windows abs paths. That is what for adhoc _p was introduced here and
worked in other tests.  With RF to use _p insteadof _path_ test started
to pass on Windows.
adswa
adswa approved these changes Aug 20, 2022
View reviewed changes
Copy link
Member

@adswa adswa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now its green! :)

@yarikoptic yarikoptic merged commit 92541dd into datalad:maint Aug 20, 2022
@yarikoptic yarikoptic deleted the bf-path_is_subpath branch October 14, 2022 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adswa adswa adswa approved these changes

Assignees
No one assigned
Labels
semver-tests Changes only affect tests, no impact on version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yarikoptic @adswa