Repository navigation
TST: Fix parallel data download test - #7575
Conversation
|
Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Looks good, but just one small comment below
| (mirror_url in c_urls) and (main_url not in c_urls)) | ||
| # Only test the rest if main URL did not timed out | ||
| if main_alive: | ||
| download_file(mirror_url, cache=True) # Cache mirror copy as well |
There was a problem hiding this comment.
What if the main server is up but the mirror isn't? Won't this fail?
There was a problem hiding this comment.
Yes. I also thought about moving mirror test out into a separate function, but that does not solve the "test failing when main/mirror fail" problem; something is still going to fail but just not this function.
I thought about marking as xfail when things fail here but that defeats the purpose of the test, no? I guess, we should step back and ask... what do we really want to test. Shall we use mock if we don't want to ever deal with flaky main/mirror URLs?
There was a problem hiding this comment.
I guess this relates to the unit test x integration test discussion, right? If the scope of the test is just to be an unit test, in my perspective it should use mock to avoid reaching external sources. You should be able to test if the method tries to reach mirror/cache using mocks.
|
@astrofrog , I split the tests and refactored part of it to not require network access. What do you think now? |
|
OK merging now. I'll submit a v2.x backport shortly. Thanks! |
TST: Fix parallel data download test
4 participants
Fix #7573
Close #6061
TODO: