Skip to content

Conversation

NicolasHug
Copy link
Member

This PR ports the test_internet.py tests to pytest.

It also adds the pytest-mock test-time dependency. I'll comment below for why this is probably preferable, but I'm happy to hear y'all thoughts on this.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

LGTM to add the extra test dependency on pytest-mock, if it's governed by pytest

@NicolasHug NicolasHug merged commit 2786a12 into pytorch:master Jun 14, 2021
@github-actions
Copy link

Hey @NicolasHug, @fmassa!

You approved or merged this PR, but no labels were added.

@NicolasHug
Copy link
Member Author

Thanks for the reviews!
I'll try to import this soon in fbcode to make sure there's no dependency issue, hopefully later today.

BTW, now that we rely on pytest, we could put back those tests where they used to be instead of having a dedicated file for internet tests. We could just mark them with a needs_internet mark, and just bypass those tests in fbcode, similar to what is done with the needs_cuda mark in https://github.com/pytorch/vision/pull/4025/files

@fmassa
Copy link
Member

fmassa commented Jun 14, 2021

@NicolasHug everything should be fine in fbcode because there is no TARGET for test_internet.

I would be ok moving the tests back to where they were originally, although given that they are for now self-contained to just the download functionality, I would be fine leaving them here for now

facebook-github-bot pushed a commit that referenced this pull request Jun 15, 2021
…cy (#4032)

Reviewed By: mthrok

Differential Revision: D29105974

fbshipit-source-id: 74baaf55f52d5f064d193ce55e5a6ab4fd45b51c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants