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

Fix mock in test_cached_files_are_used_when_internet_is_down #18804

Merged
merged 1 commit into from
Aug 29, 2022

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Aug 29, 2022

What does this PR do?

Fix the CI that is currently breaking because of the 0.9.1 patch release of huggingface_hub.

Problem is that we are looking at the response from the server when having a HTTPError. In the tests, the response is mocked which makes response.json() a mock instead of a dictionary. I now set it to {} which means an empty response from the server.

See slack thread (internal link) for more context.

Expected result

The CI should now pass correctly.

@Wauplin Wauplin requested review from ydshieh and LysandreJik August 29, 2022 10:22
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Wauplin

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 29, 2022

The documentation is not available anymore as the PR was closed or merged.

@ydshieh
Copy link
Collaborator

ydshieh commented Aug 29, 2022

Thank you, @Wauplin . As my knowledge of Mock is also mocked, here is my noob question:

The response object here

server_message = response.json().get("error", None)

is the response_mock in

response_mock.json.return_value = {}

?

And if we don't set return_value as done in this PR, response.json()["error"] is also a mocked object? (Is it response_mock itself, or another one).

@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 29, 2022

Hi @ydshieh, I'll try to explain the PR a bit.

In the tests, we are defining a Mock object response_mock. In python, Mock are objects on which you can call any attribute and it will return a new mock object. Since object methods are also attributes, they are mocked as well.
Here is a short example to understand it better:

>>> from unittest.mock import Mock
>>> my_mock = Mock()

# the mock we defined
>>> my_mock 
<Mock id='140556566369952'>

# `foo` is also a mock we different id
>>> my_mock.foo 
<Mock name='mock.foo' id='140556566370816'>

# `foo` can be called as a function and return a new mock with different id
>>> my_mock.foo() 
<Mock name='mock.foo()' id='140556566424112'>

# `.foo()` is a mock so you can call anything from it
>>> my_mock.foo().bar 
<Mock name='mock.foo().bar' id='140556530612016'>

# now we set a custom return value for `.foo()`
>>> my_mock.foo.return_value = 4 

 # `.foo` still a mock
>>> my_mock.foo
<Mock name='mock.foo' id='140556566370816'>

# `.foo()` is now a "normal" value, here the integer 4
>>> my_mock.foo() 
4

# not a mock so cannot call `.bar` from `4`
>>> my_mock.foo().bar # not a mock so cannot call `.bar` from `4`
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'bar'

The goal of a mock is that almost any manipulation done on it will not fail. It will continue to pass mocked values which in the end you can test. There are a few tweaks you can do on a mock like raising an Error (which is done with the side_effect in the implemented test or returning a special value (like in this PR)


To come back to you initial question, when "requests.request" is patch, it means if in this context requests.request is called then the actual code will not be returned but instead the response_mock will be returned. This is done so that the actual call is not done.

So yes, when in huggingface_hub there is a response.json().get("error", None), it is actually response_mock.json().get("error", None). Since a return value is set to response_mock.json(), this is equivalent to do {}.get("error", None) (which is None).

        with mock.patch("requests.request", return_value=response_mock) as mock_head:
            _ = BertConfig.from_pretrained("hf-internal-testing/tiny-random-bert")

And finally to be complete, mock objects are also nice because they count how much calls have been made to them. So at the end of test the mock_head.assert_called() means that we expect that the mock has been called at least once otherwise it would fail.

Hope this help you understand what mocks are doing :)

@Wauplin Wauplin closed this Aug 29, 2022
@Wauplin Wauplin reopened this Aug 29, 2022
@Wauplin
Copy link
Contributor Author

Wauplin commented Aug 29, 2022

@ydshieh I answered to your question in the previous comment but posted it when it was half-written by mistake. Now the explanation is complete :)
Please let me know if you have other questions. Otherwise I let you merge it.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Thank you @Wauplin, for the fix as well as for unmocking my Mock knowledge 😃

@ydshieh ydshieh changed the title Fix mock in 'test_cached_files_are_used_when_internet_is_down' tests … Fix mock in test_cached_files_are_used_when_internet_is_down Aug 29, 2022
@ydshieh ydshieh merged commit 169b8cd into huggingface:main Aug 29, 2022
@Wauplin Wauplin deleted the wauplin-fix-mock-issue-in-ci branch August 29, 2022 14:16
oneraghavan pushed a commit to oneraghavan/transformers that referenced this pull request Sep 26, 2022
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.

4 participants