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
Validate enforce
parameters
#1265
Conversation
Codecov ReportBase: 95.73% // Head: 95.74% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## V1.0.0.dev #1265 +/- ##
===========================================
Coverage 95.73% 95.74%
===========================================
Files 47 47
Lines 3685 3691 +6
===========================================
+ Hits 3528 3534 +6
Misses 157 157
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -765,7 +765,7 @@ def test_transform(self): | |||
try: | |||
[uuid.UUID(u) for c, u in out['b#c'].items()] | |||
except ValueError: | |||
pytest.fail('ValueError') | |||
pytest.fail('ValueError') # noqa: PT009 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8-pytest-style
update doesn't let you use these anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be in a try/except block? Wouldn't the ValueError
cause the test to fail anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this line usually get hit? I'm not sure why we need this anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frances-h I believe it's for clarity: it's weird to just create a list and not use it. This makes it clear we are expecting uuid.UUID()
to not raise a ValueError.
@amontanez24 It never gets reached, since the test (hopefully) never fails. The list itself should stay there though, since we need to verify that every value was correctly converted into an id.
I really don't think this matters either way, I'll just delete it as you suggested so we don't have to add the noqa
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fealho re: clarity, another option could be to add an assert
before the list. Assuming the list is not empty, it'll always pass and maybe be clearer that we want to explicitly test creating a list of UUIDs.
except ValueError: | ||
pytest.fail('ValueError') | ||
|
||
[uuid.UUID(u) for _, u in out['b#c'].items()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is just trying to check that the outputs are indeed all UUIDs, which is what they're supposed to be. Tbh I don't know if that is necessary for a unit test. At this point, the map of uuids to combinations should have already been made and we really just need to assert that the values are from that map like expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve #1242