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

Remove unittest (was: Refactor test suite) #161

Merged
merged 8 commits into from
Oct 11, 2017

Conversation

igneus
Copy link
Contributor

@igneus igneus commented Oct 9, 2017

I've started working on #98 . I open this PR in order to allow maintainers to review continuously if they wish so.

* quite a few cases have been lost along the way, find out where!
self.assertEqual(
str(d),
assert(
str(d) ==
"<%s('key': 'one', 'key': 'two')>" % cls.__name__)
Copy link
Member

Choose a reason for hiding this comment

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

You'd better do expected = "<%s('key': 'one', 'key': 'two')>" % cls.__name__ one line above instead of having multiline expression for the sake of readability


def test_eq(self):
d1 = self.make_dict(Key='val')
d2 = self.make_dict(KEY='val')

self.assertEqual(d1, d2)
assert d1 == d2


class TestPyMultiDictProxy(_TestProxy, unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to eliminate unittest.TestCase here as well ;)

@webknjaz
Copy link
Member

webknjaz commented Oct 9, 2017

@igneus thanks for taking this task! We'll be waiting for the completed version :)

[('key', 'one'), ('key', 'two'), ('key', 3)])
expected = [('key', 'one'), ('key', 'two'), ('key', 3)]
assert list(d.items()) == expected
assert list(d.items()) == expected # was the repetition intentional?
Copy link
Member

Choose a reason for hiding this comment

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

[('key', 'one'), ('key', 'two'), ('key', 3)])
expected = [('key', 'one'), ('key', 'two'), ('key', 3)]
assert list(d.items()) == expected
assert list(d.items()) == expected # was the repetition intentional?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asvetlov I stumbled upon this repeated assertion. Either it's redundant or I don't understand it's meaning. It seems to have originated at f84e6bc . Should it stay or should it go?

@igneus
Copy link
Contributor Author

igneus commented Oct 10, 2017

Unittest is gone. Should I continue the "more serious" test refactoring in this PR, or do you prefer to review and merge this step before going on?

@webknjaz
Copy link
Member

@igneus it is much easier to keep track of small changes. I'd be glad to merge this one after review and proceed with separate PR

@igneus igneus changed the title Refactor test suite [WIP, do not merge yet] Refactor test suite Oct 10, 2017
@igneus igneus changed the title Refactor test suite Remove unittest (was: Refactor test suite) Oct 10, 2017
@codecov
Copy link

codecov bot commented Oct 10, 2017

Codecov Report

Merging #161 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #161   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         329    329           
=====================================
  Hits          329    329

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 9d2c170...e251812. Read the comment docs.

[('key', 'one'), ('key', 'two'), ('key', 3)])
expected = [('key', 'one'), ('key', 'two'), ('key', 3)]
assert list(d.items()) == expected
assert list(d.items()) == expected # was the repetition intentional?
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Honestly I don't remember.
You could consider it as a test for stable items order but better to have a separate test for the case.
Let's just remove the line.

@webknjaz
Copy link
Member

continuous-integration/appveyor/branch — Waiting for status to be reported

it's stuck for hours (and it's not the first time this issue appears)

@asvetlov can we skip this check?

@asvetlov
Copy link
Member

Sure

@asvetlov asvetlov merged commit e50b140 into aio-libs:master Oct 11, 2017
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.

3 participants