Replace asserts with ValueErrors #5275
Conversation
Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com>
Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com>
💊 CI failures summary and remediationsAs of commit d79b512 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
test/test_backbone_utils.py
Outdated
| y = resnet_fpn_backbone(backbone_name=backbone_name, pretrained=False)(x) | ||
| assert list(y.keys()) == ["0", "1", "2", "3", "pool"] | ||
|
|
||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
As a convention, Please append the error message
| with pytest.raises(ValueError): | |
| with pytest.raises(ValueError, "Whatever error message it will raise"): |
There was a problem hiding this comment.
yes you left that comment on previous PR,
There was a problem hiding this comment.
This seems to be failing. Should we add match argument as per https://docs.pytest.org/en/6.2.x/reference.html?highlight=raises#pytest.raises
Here is a simpler test I wrote that fails too with the same error TypeError: 'Sth' object (type: <class 'str'>) must be callable:
import pytest
def checkme(p):
raise ValueError("Sth")
@pytest.mark.parametrize("mypar", (1,2,3))
def testMethod(mypar):
with pytest.raises(ValueError, "Sth"):
checkme(mypar)
There was a problem hiding this comment.
Resolved. It is happy with the match argument
test/test_models_detection_utils.py
Outdated
| assert ret == 3 | ||
| # can't go beyond 5 | ||
| with pytest.raises(AssertionError): | ||
| with pytest.raises(ValueError): |
|
@piyush01123 I have added error messages in tests; could you merge that pr(#5264 ) in this pr? |
|
Okay; you have added those messages; then I will close my PR! Thanks. |
|
ig there are some lint issues;
|
|
hey I am sorry I am so much slow at this. I am not very fast with linting and testing. I am trying to resolve everything you guys have raises though. |
i guess there are no lint issues after the recent PR. |
test/test_models_detection_utils.py
Outdated
| assert ret == 3 | ||
| # can't go beyond 5 | ||
| with pytest.raises(AssertionError): | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
This is resolved now I think
|
Nice work! Thanks a lot to @abhi-glitchhg and @piyush01123 for great collaboration. And congratulation to @piyush01123 for first contribution 🎉 |
Summary: * replace assert with valueerror * pytest should raise ValueError not AssertionError * minor edit * raise assert changed to raise valueerror in test * Update torchvision/models/detection/backbone_utils.py * Update torchvision/models/detection/backbone_utils.py * minor edits * minor edits * added one test * added another test * added another test * test for mobilenet * ufmt formatting * cant have unused variables * suggested changes * minor edit * corrected bug pointed out by datumbox * corrected bug pointed out by datumbox * bug correction and shorten msg * ufmt stuff * resolved last comment Reviewed By: kazhang Differential Revision: D33927485 fbshipit-source-id: 152522dca213ea1b16ca5fb8f12fc21f803233dd Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com> Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com> Co-authored-by: Abhijit Deo <72816663+abhi-glitchhg@users.noreply.github.com> Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com> Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com>
Contains commits from @abhi-glitchhg and me on #5228