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.

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

BF: use '' as default (not None) while checking for a key not to be a test key #4552

Merged
merged 7 commits into from May 20, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented May 19, 2020

I just saw it in https://github.com/datalad/datalad-extensions/runs/665491656?check_suite_focus=true
which I believe failed for other reasons (but may be not), that there was a traceback
to this line. And indeed above we might ask for a 'key' or a 'file', so hypothetically
there might be no 'key' field. I have not really analyzed situation on how we have not
hit it before and how it really emerges in that failure while testing against
bleeding edge git annex

… test key

I just saw in https://github.com/datalad/datalad-extensions/runs/665491656?check_suite_focus=true
which I believe failed for other reasons (but may be not), that there was a traceback
to this line.  And indeed above we might ask for a 'key' or a 'file', so hypothetically
there might be no 'key' field.  I have not really analyzed situation on how we have not
hit it before and how it really emerges in that failure while testing against
bleeding edge git annex
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

oh, on the most recent run https://github.com/datalad/datalad-extensions/runs/687454738?check_suite_focus=true it is the only error which causes the failure! I will try to check out what is that record now...

edit: with this fix test still would fail but differently:

  File "/home/yoh/proj/datalad/datalad-maint/datalad/support/annexrepo.py", line 2384, in whereis
    return {
  File "/home/yoh/proj/datalad/datalad-maint/datalad/support/annexrepo.py", line 2386, in <dictcomp>
    : self._whereis_json_to_dict(j)
  File "/home/yoh/proj/datalad/datalad-maint/datalad/support/annexrepo.py", line 2105, in _whereis_json_to_dict
    assert (j.get('success', True) is True)
AssertionError

so clearly something changed in git-annex behavior. heh.

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 19, 2020

I can trigger it locally. I'll try to bisect it.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

no need -- I recalled what it is about!

kyleam
kyleam approved these changes May 19, 2020
Copy link
Contributor

@kyleam kyleam left a comment

The change itself looks obviously correct. I was confused what this '.this-is-a-test-key' bit was about. It looks like annex can leave that behind in some situations: 9d18fd2. I wonder if that's still the case.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

I bet (didn't check explicitly) it is due to 8.20200501-55-g39d7e6dd2
where git-annex stopped "sanitizing" - in the middle of the filenames for urls it downloads from the web.

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 19, 2020

I bet (didn't check explicitly) it is due to 8.20200501-55-g39d7e6dd2

FWIW the bisect pointed to the related cabbc91b1 (addurl, importfeed: Allow '-' in filenames, as long as it's not the first character, 2020-05-11). (edit: for comparison, that's 8.20200501-53-gcabbc91b1)

@codecov
Copy link

@codecov codecov bot commented May 19, 2020

Codecov Report

Merging #4552 into maint will increase coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##            maint    #4552   +/-   ##
=======================================
  Coverage   90.14%   90.15%           
=======================================
  Files         275      275           
  Lines       37101    37099    -2     
=======================================
- Hits        33446    33445    -1     
+ Misses       3655     3654    -1     
Impacted Files Coverage Δ
datalad/distribution/tests/test_install.py 99.79% <ø> (+0.20%) ⬆️
datalad/support/annexrepo.py 86.30% <ø> (ø)
datalad/customremotes/base.py 84.21% <50.00%> (+0.06%) ⬆️
datalad/customremotes/tests/test_datalad.py 95.55% <83.33%> (-4.45%) ⬇️
...atalad/interface/tests/test_add_archive_content.py 100.00% <100.00%> (ø)
datalad/utils.py 84.07% <0.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bce4caf...32e6cbf. Read the comment docs.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

oh, thanks! I just pushed, but will rewrite and force push with the correct exact version! ;)

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented May 19, 2020

done... also "polluted" this PR with more commits stripping away some ancient annex versions checks where version is below the min version we currently support. It all must be green! ;)

@@ -30,14 +31,19 @@ def check_basic_scenario(url, d):
# TODO skip if no boto or no credentials
get_test_providers(url) # so to skip if unknown creds

# git-annex got a fix where it stopped replacing - in the middle of the filename
filename = '3versions%sallversioned.txt' % (
'_' if external_versions['cmd:annex'] < '8.20200501+git53-gcabbc91b1' else '-'
Copy link
Contributor

@kyleam kyleam May 19, 2020

Choose a reason for hiding this comment

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

This is fine, though I think it'd be even better to check whether a file with one of the two names exists after the add_urls call. The current approach means this test would fail for me with a local git-annex build after 8.20200501 but before that commit, where cmd:annex would be something like 8.20200502-g[xdigit]+

Copy link
Member Author

@yarikoptic yarikoptic May 19, 2020

Choose a reason for hiding this comment

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

Heh.. I thought to use glob first... Ok, will adjust later while also checking that expected name is popping out for versions outside of the development range

Copy link
Member Author

@yarikoptic yarikoptic May 19, 2020

Choose a reason for hiding this comment

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

Since I do want to have at least this single test to check for expected annex behavior

@kyleam
Copy link
Contributor

@kyleam kyleam commented May 19, 2020

with more commits stripping away some ancient annex versions checks where version is below the min version we currently support

Thanks.

…ases

Also moved out some annex. calls outside of swallow_outputs -- we do not test
them and I do not expect them to flood the screen much
@kyleam kyleam merged commit e8dfe59 into datalad:maint May 20, 2020
10 of 11 checks passed
@yarikoptic yarikoptic deleted the bf-no-key branch May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants