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

Refactor unit tests #167

Merged
merged 3 commits into from
Oct 23, 2017

Conversation

akhomchenko
Copy link
Contributor

@akhomchenko akhomchenko commented Oct 11, 2017

Changes:

  • merged test_instantiate__from_arg0 and test_instantiate__from_arg0_dict
  • tests from _BaseMutableMultiDictTests are at TestMutableMultiDict
  • TestNonProxyCIMultiDict#test_extend_with_istr was moved to TestMutableMultiDict
  • _TestProxy/_TestCIProxy#test_copy was moved to test_proxy_copy

Questions:

  • test_instantiate__with_kwargs: should we rely on order and drop sorted?
  • test_instantiate__empty: is duplication done by purpose?
  • test_items__contains: is duplication done by purpose?
  • test_get duplicates test_getone
  • _CIMultiDictTests was inherited from _Root and not _BaseTest
  • test__iter__types was done only for non-ci dicts. Should I add CI classes?

@webknjaz
Copy link
Member

I'd give OPTIONAL_CYTHON approach a try.

Regarding the rest of refactoring, let's invite @igneus to try splitting the scope somehow, since he was going to do additional PR as well.

@codecov
Copy link

codecov bot commented Oct 11, 2017

Codecov Report

Merging #167 into master will decrease coverage by 0.3%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage     100%   99.69%   -0.31%     
==========================================
  Files           4        4              
  Lines         329      329              
==========================================
- Hits          329      328       -1     
- Misses          0        1       +1
Impacted Files Coverage Δ
multidict/_multidict_py.py 99.66% <0%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b015c...92081d6. Read the comment docs.

@igneus
Copy link
Contributor

igneus commented Oct 12, 2017

@webknjaz

Regarding the rest of refactoring, let's invite @igneus to try splitting the scope somehow, since he was going to do additional PR as well.

I don't think I have anything helpful to say right now. "Splitting the scope" is a task quite a few levels above my current level of understanding of the code in question.

@webknjaz
Copy link
Member

@igneus would you wait for @gagoman to finish this PR then?

@webknjaz
Copy link
Member

@gagoman could you please apply your changes to existing files rather than creating new

@igneus
Copy link
Contributor

igneus commented Oct 12, 2017

@webknjaz

@igneus would you wait for @gagoman to finish this PR then?

Sure. To be honest: I had no real plans concerning the "true refactoring" phase of the work (beyond removing unittest) and my attempt to approach it before I learned about @gagoman 's work on this PR hasn't resulted in anything worth a commit -- stuff just got more complicated than it had been originally. It seems I can't really "think the pytest way" yet when it comes to fixtures and parametrization.

@asvetlov
Copy link
Member

@gagoman please not only add new tests but drop obsolete unittest styled ones.
Otherwise it's hard to understand how many tests from test_multidict.py are still not converted

@akhomchenko
Copy link
Contributor Author

@asvetlov I will do.
@webknjaz yes sure, it is WIP, I will cleanup before final PR.

* moved multidict tests to test_mutable_multidict
* moved types tests to test_types
@akhomchenko
Copy link
Contributor Author

Feel free to review. This is quite a big change though.

Now there would be more tests, as previously CIMultiDict tests were inherited not from BaseTest class.

I have some questions that I've placed in PR body.

CIMultiDict,
istr
)
def chained_ctor(_multidict_module, impls):
Copy link
Member

Choose a reason for hiding this comment

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

I cannot guess what this function name stands for. Could you plz think of its improvement? Explicit is better, than implicit.

Oh.. and it would be great it you could add docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will do. Any preference on name?

@webknjaz
Copy link
Member

I'll review it closer a bit later, but at glance it looks cleaner now :)

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks good but replaced tests should be deleted.

@akhomchenko
Copy link
Contributor Author

@asvetlov could you please elaborate? Which tests? There are no duplicates atm.

@asvetlov
Copy link
Member

@gagoman nevermind. Missed collapsed file

@asvetlov asvetlov merged commit 1889422 into aio-libs:master Oct 23, 2017
@asvetlov
Copy link
Member

thanks!

@akhomchenko akhomchenko deleted the feature/ref-multidict-tests branch October 30, 2017 16:34
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