Skip to content

Fix test to avoid use of cache internals - #10430

Closed
aarchiba wants to merge 3 commits into
astropy:masterfrom
aarchiba:fix_10401
Closed

Fix test to avoid use of cache internals#10430
aarchiba wants to merge 3 commits into
astropy:masterfrom
aarchiba:fix_10401

Conversation

@aarchiba

@aarchiba aarchiba commented Jun 2, 2020

Copy link
Copy Markdown
Contributor

Description

Update the test in astropy/coordinates/tests/test_name_resolve.py so that it does not use the internals of the caching mechanism. Probably fixes #10401 ? Or at least should clarify what is going on.

@aarchiba aarchiba mentioned this pull request Jun 3, 2020
Merged
@pllim
pllim requested review from adrn and pllim June 3, 2020 16:24
@pllim pllim added this to the v4.2 milestone Jun 3, 2020
@aarchiba

aarchiba commented Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

I think I'm going to need to merge this into #10437 to fix a spurious test failure.

aarchiba added a commit to aarchiba/astropy that referenced this pull request Jun 3, 2020
@aarchiba
Fix bogus test
e34afc1 
This is what is in astropy#10430
@pllim

pllim commented Jun 4, 2020

Copy link
Copy Markdown
Member

@aarchiba , should I review (and merge if it looks okay) or hold off on it and wait for the other PR? Please advise. Thanks!

@pllim

pllim commented Jun 5, 2020

Copy link
Copy Markdown
Member

Also maybe @dhomeier can confirm whether or not this really fixes things on his machine.

@aarchiba

aarchiba commented Jun 7, 2020

Copy link
Copy Markdown
Contributor Author

@aarchiba , should I review (and merge if it looks okay) or hold off on it and wait for the other PR? Please advise. Thanks!

Please hold off. It''s the same change as in the other PR so it's worth checking whether it fixes the OP's problem, but it is superseded (and required) by the other PR.

@dhomeier

dhomeier commented Jun 11, 2020

Copy link
Copy Markdown
Contributor

Also maybe @dhomeier can confirm whether or not this really fixes things on his machine.

Yes, test passes now, thanks!

adrn
adrn approved these changes Jun 12, 2020
View reviewed changes

@adrn adrn left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I don't think this needs a changelog entry. I'll let @pllim decide whether we should remove the label (since you already wrote one) or if you should remove it.

@pllim

pllim commented Jun 12, 2020

Copy link
Copy Markdown
Member

@adrn , I am holding off on review as requested by @aarchiba in #10430 (comment) .

@pllim pllim closed this in #10437 Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adrn adrn adrn approved these changes
@pllim pllim Awaiting requested review from pllim

Assignees

No one assigned

Labels

coordinates no-changelog-entry-needed testing

Projects

None yet

Milestone

v4.2

Development

Successfully merging this pull request may close these issues.

get_cached_urls() return is empty in test_name_resolve_cache

4 participants

@aarchiba @pllim @dhomeier @adrn