Skip to content

Refactor unit tests#167

Merged
asvetlov merged 3 commits intoaio-libs:masterfrom
akhomchenko:feature/ref-multidict-tests
Oct 23, 2017
Merged

Refactor unit tests#167
asvetlov merged 3 commits intoaio-libs:masterfrom
akhomchenko:feature/ref-multidict-tests

Conversation

@akhomchenko
Copy link
Copy Markdown
Contributor

@akhomchenko akhomchenko commented Oct 11, 2017 •
edited
Loading

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
Copy Markdown
Member

webknjaz commented Oct 11, 2017

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
Copy Markdown

codecov bot commented Oct 11, 2017 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Member

webknjaz commented Oct 12, 2017

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

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Oct 12, 2017

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

@igneus
Copy link
Copy Markdown
Contributor

igneus commented Oct 12, 2017 •
edited
Loading

@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
Copy Markdown
Member

asvetlov commented Oct 13, 2017

@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
Copy Markdown
Contributor Author

akhomchenko commented Oct 13, 2017

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

@webknjaz webknjaz requested review from asvetlov and webknjaz October 15, 2017 12:57
@akhomchenko akhomchenko force-pushed the feature/ref-multidict-tests branch from d4602bf to 949779e Compare October 22, 2017 16:25
@akhomchenko
Refactor unit tests to pytest style
2594ce8 
* moved multidict tests to test_mutable_multidict
* moved types tests to test_types
@akhomchenko akhomchenko force-pushed the feature/ref-multidict-tests branch from 949779e to 2594ce8 Compare October 22, 2017 16:29
@akhomchenko
Copy link
Copy Markdown
Contributor Author

akhomchenko commented Oct 22, 2017

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.

webknjaz
webknjaz reviewed Oct 22, 2017
View reviewed changes
tests/test_multidict.py
CIMultiDict,
istr
)
def chained_ctor(_multidict_module, impls):
Copy link
Copy Markdown
Member

@webknjaz webknjaz Oct 22, 2017

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
Copy Markdown
Contributor Author

@akhomchenko akhomchenko Oct 23, 2017

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
Copy Markdown
Member

webknjaz commented Oct 22, 2017

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

asvetlov
asvetlov requested changes Oct 23, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

akhomchenko commented Oct 23, 2017

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

@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Oct 23, 2017

@gagoman nevermind. Missed collapsed file

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

asvetlov commented Oct 23, 2017

thanks!

@akhomchenko akhomchenko mentioned this pull request Oct 23, 2017
Merged
@akhomchenko akhomchenko deleted the feature/ref-multidict-tests branch October 30, 2017 16:34
@akhomchenko akhomchenko mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz left review comments

@asvetlov asvetlov asvetlov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@akhomchenko @webknjaz @igneus @asvetlov