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.

Sign up for GitHub

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

Jump to bottom

Fixed several assertTrue() that were intended to be assertEqual(). #8191

Merged
merged 2 commits into from
Jul 9, 2018
Merged

Fixed several assertTrue() that were intended to be assertEqual(). #8191

merged 2 commits into from
Jul 9, 2018

Conversation

sir-sigurd
Copy link
Contributor

No description provided.

serhiy-storchaka
serhiy-storchaka reviewed Jul 9, 2018
View reviewed changes
Lib/ctypes/test/test_as_parameter.py Outdated
@@ -24,7 +24,7 @@ def test_wchar_parm(self):
f.argtypes = [c_byte, c_wchar, c_int, c_long, c_float, c_double]
result = f(self.wrap(1), self.wrap("x"), self.wrap(3), self.wrap(4), self.wrap(5.0), self.wrap(6.0))
self.assertEqual(result, 139)
self.assertTrue(type(result), int)
self.assertEqual(type(result), int)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe even assertIs?

Lib/test/test_warnings/__init__.py Outdated
@@ -149,9 +149,9 @@ def test_always(self):
self.module.filterwarnings("always", category=UserWarning)
message = "FilterTests.test_always"
self.module.warn(message, UserWarning)
self.assertTrue(message, w[-1].message)
self.assertEqual(message, w[-1].message.args[0])
Copy link
Member

Choose a reason for hiding this comment

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

There are other flaws in this test, due to which the second assertion doesn't have effect. Rewrite the test as:

    def test_always(self):
        with original_warnings.catch_warnings(record=True,
                module=self.module) as w:
            self.module.resetwarnings()
            self.module.filterwarnings("always", category=UserWarning)
            message = "FilterTests.test_always"
            def f():
                self.module.warn(message, UserWarning)
            f()
            self.assertEqual(len(w), 1)
            self.assertEqual(w[-1].message.args[0], message)
            f()
            self.assertEqual(len(w), 2)
            self.assertEqual(w[-1].message.args[0], message)

@serhiy-storchaka
Copy link
Member

Good catch!

@serhiy-storchaka serhiy-storchaka merged commit b796e7d into python:master Jul 9, 2018
@miss-islington
Copy link
Contributor

Thanks @sir-sigurd for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-8200 is a backport of this pull request to the 3.7 branch.

@miss-islington
Copy link
Contributor

Sorry, @sir-sigurd and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b796e7dcdc24ff7ec53044af041254c83a8ace21 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 9, 2018
@sir-sigurd @miss-islington
Fixed several assertTrue() that were intended to be assertEqual(). (p…
4640209 
…ythonGH-8191)

Fixed also testing the "always" warning filter.
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
@bedevere-bot
Copy link

GH-8201 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 9, 2018
@sir-sigurd @miss-islington
Fixed several assertTrue() that were intended to be assertEqual(). (p…
73a56f5 
…ythonGH-8191)

Fixed also testing the "always" warning filter.
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
@sir-sigurd sir-sigurd deleted the assert-true branch July 9, 2018 15:29
miss-islington added a commit that referenced this pull request Jul 9, 2018
@miss-islington @sir-sigurd
Fixed several assertTrue() that were intended to be assertEqual(). (G…
958a25e 
…H-8191)

Fixed also testing the "always" warning filter.
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
miss-islington added a commit that referenced this pull request Jul 9, 2018
@miss-islington @sir-sigurd
Fixed several assertTrue() that were intended to be assertEqual(). (G…
13d1025 
…H-8191)

Fixed also testing the "always" warning filter.
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Jul 9, 2018
@sir-sigurd @serhiy-storchaka
[2.7] Fixed several assertTrue() that were intended to be assertEqual…
9adb312 
…(). (pythonGH-8191)

Fixed also testing the "always" warning filter..
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
@bedevere-bot
Copy link

GH-8202 is a backport of this pull request to the 2.7 branch.

serhiy-storchaka added a commit that referenced this pull request Jul 9, 2018
@serhiy-storchaka @sir-sigurd
[2.7] Fixed several assertTrue() that were intended to be assertEqual…
789f95a 
…(). (GH-8191) (GH-8202)

Fixed also testing the "always" warning filter.
(cherry picked from commit b796e7d)

Co-authored-by: Sergey Fedoseev <fedoseev.sergey@gmail.com>
@serhiy-storchaka serhiy-storchaka removed their assignment Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip issue skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sir-sigurd @serhiy-storchaka @miss-islington @bedevere-bot @the-knights-who-say-ni