-
-
Notifications
You must be signed in to change notification settings - Fork 101
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
[WIP] Issue#98: refactor test suite #294
[WIP] Issue#98: refactor test suite #294
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tests/test_multidict_.py
Outdated
class TestContents: | ||
|
||
@pytest.fixture(autouse=True) | ||
def _autoassign_md_to_d(self, md_w_multivalue_per_key): |
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.
I like the parametrization approach but this particular fixture looks like a hack.
Instead of self.d
we can always use a fixture for multidict instance explicitly.
This allows getting rid of self.d
entirely.
self.d
is necessary for unittest
based approach but redundant for pytest
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.
Also, classes are normally not necessary. If you need namespacing, flat is better than nested: just use separate test modules :)
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.
Agree but I don't insist.
Can live with classes if they are already exist.
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.
Yeah, but classes often implicitly encourage ppl to have state. Which might lead to tests depending on each other. Why rely on human reviewers to catch those when simple rules are more effective in terms of discouraging this? :)
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.
Is getting rid of classes the suggestion or requirement?
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.
Suggestion. A requirement would need actual linting implementation.
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.
(But still, it's also: simple is better than complex)
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.
@webknjaz regarding classes -> modules part:
I mostly agree but it is hard to split something in modules when you don't actually know all parts.
Like here I am still in progress figuring out the layout and all needed fixtures.
When it is settled - I will probably come back to that thought.
tests/test_multidict_.py
Outdated
assert list(d.items()) == [] | ||
|
||
assert cls() != list() | ||
with pytest.raises(TypeError, match='\(2 given\)'): |
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.
please use raw-string literals instead of escaping everything
hey @asvetlov , just a friendly ping. |
If this is still relevant after #915, please recreate the PR with those fixtures. |
Disclaimer: work in progress. I need initial review to proceed.
Main points:
[MultiDict, CIMultiDict, MultiDictProxy, CIMultiDictProxy]
) are pre-created and used for tests which concern not instantiation of those classes themselves(unfortunately, pytest so far [doesn't support fixtures in pytest.mark.parametrize] )(Using fixtures in pytest.mark.parametrize pytest-dev/pytest#349)
useautouse=True
class level hack (since we can't use__init__
) to be able to useself.d
instead of long fixture names.Since my goal is restructuring - I didn't touch asserts themselves. I think that such comments can be addressed after the fact.