[MRG] Backports msg in assert_raises and assert_raises_regex #9536

thechargedneutron · 2017-08-12T10:38:30Z

Reference Issue

What does this implement/fix? Explain your changes.

I added a new directory named modifiedunittest which backports msg.

Any other comments?

I have not removed the extra codes of the pasted module, will be done upon validation of my task.
This should work with Python 2.6 as well. If it fails, necessary improvement will be done.
Thanks

thechargedneutron · 2017-08-12T11:12:01Z

After having a look at the error log, which states ImportError, what all changes do I need to make in order to recognise the package I used.
One thing I observed was to add config.add_subpackage('modifiedunittest') in sklearn.utils.setup.py . Any other location where I must add something?

…it-learn into Secondary

jnothman · 2017-08-13T14:12:02Z

sklearn/utils/modifiedunittest/case.py

+    return isinstance(expected, type) and issubclass(expected, basetype)
+
+
+class _AssertRaisesContext(_AssertRaisesBaseContext):


I think we basically just want this (perhaps with some changes for compatibility), and nothing else.

Okay, I'll remove all the unnecessary packages once I am sure the whole works. I am getting ImportError on writing import sklearn.utils.modifiedunittest . Please help me in importing it correctly. I'll remove the extra code here.

thechargedneutron · 2017-08-13T14:23:11Z

So, now I realise that we don't need to import the modules and all. We can just copy paste the codes in testing.py itself since you pointed out that there's only a few things to be added. Is this a good idea or importing from other file is better.

…into Secondary

thechargedneutron · 2017-08-13T20:59:41Z

I guess now we have the minimum amount of code in sklearn.utils.testing.py. The extra module added is removed. Hope this works fine.
Sorry for so many commits.

jnothman · 2017-08-13T23:11:44Z

many commits is not a problem. I'll try look soon

…

On 14 Aug 2017 6:59 am, "Kumar Ashutosh" ***@***.***> wrote: I guess now we have the minimum amount of code in sklearn.utils.testing.py. The extra module added is removed. Hope this works fine. Sorry for so many commits. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9536 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz64ahlMLxhQ7e0s6KoPcW_idl4Kxjks5sX2O-gaJpZM4O1YVM> .

thechargedneutron · 2017-08-15T06:39:37Z

@jnothman Does it work or there's something more to be done here?
The codecov patch test fails. But I don't think we need the test function of the functions added by me. I need your opinion in this.

amueller · 2017-08-15T20:14:39Z

sklearn/utils/testing.py

           "assert_approx_equal", "SkipTest"]


+def _is_subtype(expected, basetype):


Maybe it would be better to put this in a separate file like _unittest_backport.py or something?

Okay, I'll make a new file for this.

amueller · 2017-08-15T20:16:24Z

I think we should have a small test for this.

thechargedneutron · 2017-08-15T20:42:01Z

Is the backport implementation working? I wanted to know this. :)
I will add tests as soon as possible.

…into Secondary

thechargedneutron · 2017-08-15T22:48:56Z

I think I need some help in making the test functions.
In python's source code, I found the following code:
Is this the appropriate test case for _formatMessage function which is there in class TestCase_new ?
Kindly help as I am not sure about the test functions.

class TestLongMessage(unittest.TestCase):
    def setUp(self):
        class TestableTestFalse(unittest.TestCase):
            longMessage = False
            failureException = self.failureException

            def testTest(self):
                pass

        class TestableTestTrue(unittest.TestCase):
            longMessage = True
            failureException = self.failureException

            def testTest(self):
                pass

        self.testableTrue = TestableTestTrue('testTest')
        self.testableFalse = TestableTestFalse('testTest')

    def test_formatMsg(self):
        self.assertEqual(self.testableFalse._formatMessage(None, "foo"), "foo")
        self.assertEqual(self.testableFalse._formatMessage("foo", "bar"), "foo")

        self.assertEqual(self.testableTrue._formatMessage(None, "foo"), "foo")
        self.assertEqual(self.testableTrue._formatMessage("foo", "bar"), "bar : foo")

        # This blows up if _formatMessage uses string concatenation
        self.testableTrue._formatMessage(object(), 'foo')

jnothman

Thanks.

I don't think TestLongMessage is testing assertRaises.

jnothman · 2017-08-15T23:11:51Z

sklearn/utils/_unittest_backport.py

@@ -0,0 +1,173 @@
+import re


This should have attribution to where it is backported from (and the copyright notice)

jnothman · 2017-08-15T23:12:27Z

sklearn/utils/_unittest_backport.py

+        return True
+
+
+class TestCase_new(object):


If you inherit from unittest.TestCase you can use this alone as the dummy, I think

Something like this? class TestCase_new(unittest.TestCase):. And then we can remove the redundant functions?

This somehow gives error. It says ValueError: no such test method in <class 'TestCase_new'>: runTest . I looked for the error in StackOverflow and found this: https://stackoverflow.com/questions/2090479/valueerror-no-such-test-method-in-class-myapp-tests-sessiontestcase-runtes but could not make use of it. Can you suggest me how to achieve this?

jnothman · 2017-08-15T23:15:22Z

to test you want something like:

def test_assert_raises_msg():
    with assert_raises_regex(AssertionError, 'Hello world'):
        with assert_raises(ValueError, msg='Hello world'):
            # Nothing is raised. Assertion should fail.
            pass

thechargedneutron · 2017-08-16T02:59:55Z

@jnothman Okay, so I was under the impression that I have to add tests for the functions in _unittest_backport.py and not for assert_raises . If only I add tests for assert_raises_with_msg, the codecov/patch will still fail, isn't it?

jnothman · 2017-08-16T03:11:46Z

Yes, you won't get good codecov coverage. That's okay for a backport.

…

On 16 August 2017 at 12:59, Kumar Ashutosh ***@***.***> wrote: @jnothman <https://github.com/jnothman> Okay, so I was under the impression that I have to add tests for the functions in _unittest_backport.py and not for assert_raises . If only I add tests for assert_raises_with_msg, the codecov/patch will still fail, isn't it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9536 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz61VbBbmWBS2njUIXsHYQlCHgcaeDks5sYlssgaJpZM4O1YVM> .

…into Secondary

jnothman · 2017-08-16T10:10:25Z

sklearn/utils/_unittest_backport.py

+        return True
+
+
+class TestCase_new(unittest.TestCase):


you could just call the new class TestCase. Certainly, it shouldn't have an underscore in a class name.

Okay, I'll do that. somehow I am not able to use a single dummy variable, initializing unittest.Test case('init') inside new TestCase gives an error saying the 'init' function take 2 arguments, 4 given. Please look into it

…into Secondary

thechargedneutron · 2017-08-16T16:29:51Z

So, I have made all the changes suggested so far, added test_assert_raises_with_msg , added copyright in _unittest_backport.py and also imported unittest module in my backported class.
Kindly review the changes and suggest if anything else is required.

amueller · 2017-08-16T19:25:12Z

LGTM

jnothman · 2017-08-16T23:05:03Z

LGTM, thanks!

thechargedneutron · 2017-08-17T09:25:42Z

Thanks a lot, this was a very good learning experience, will continue to contribute :)

jnothman · 2017-08-17T10:43:11Z

Glad to hear it!

…

On 17 August 2017 at 19:25, Kumar Ashutosh ***@***.***> wrote: Thanks a lot, this was a very good learning experience, will continue to contribute :) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#9536 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_m4L9GuyFfLeWyV8D_D8kJiS03Zks5sZAcYgaJpZM4O1YVM> .

…learn#9536) * Added modifiedunittest * Backports msg in assertRaises and assertRaisesRegexp * Import statement corrected * Corrected import statement * Added module name in utils.setup.py * Removed Extra modules * Reordered class * _is_subtype added * Missing import added * _formatMessage added * missing variables added * Remove PEP8 failures * Removed safe_repr * _unittest_backport.py added * Import statement corrected * Added copyright * Syntax Error removed * Error removed * runTest function added * Tests added * __init__ added * Import added

thechargedneutron added 4 commits August 12, 2017 15:54

Added modifiedunittest

301d4f9

Backports msg in assertRaises and assertRaisesRegexp

a0d0621

Import statement corrected

7b8bb5e

Corrected import statement

ba7f9ee

thechargedneutron added 2 commits August 13, 2017 01:48

Added module name in utils.setup.py

b4e8aff

Merge branch 'Secondary' of https://github.com/thechargedneutron/scik…

aef20b6

…it-learn into Secondary

jnothman reviewed Aug 13, 2017

View reviewed changes

thechargedneutron added 9 commits August 13, 2017 20:21

Removed Extra modules

dff1952

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

942f5cf

…into Secondary

Reordered class

7e05921

_is_subtype added

d1a3258

Missing import added

04bc4d4

_formatMessage added

24edba5

missing variables added

64c4cf3

Remove PEP8 failures

683d74a

Removed safe_repr

d7173e2

amueller reviewed Aug 15, 2017

View reviewed changes

thechargedneutron added 3 commits August 16, 2017 02:18

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

bc7d59d

…into Secondary

_unittest_backport.py added

89b8c1d

Import statement corrected

ed740b8

jnothman reviewed Aug 15, 2017

View reviewed changes

thechargedneutron added 4 commits August 16, 2017 09:17

Added copyright

7eeffee

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

853fa05

…into Secondary

Syntax Error removed

3570cd7

Error removed

2b751d2

jnothman reviewed Aug 16, 2017

View reviewed changes

thechargedneutron added 5 commits August 16, 2017 18:21

runTest function added

ea1d3d3

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

1c2aa67

…into Secondary

Tests added

8abbba7

__init__ added

bd8f10c

Import added

22511a8

amueller changed the title ~~[WIP] Backports msg in assert_raises and assert_raises_regex~~ [MRG] Backports msg in assert_raises and assert_raises_regex Aug 16, 2017

jnothman merged commit 2b9405f into scikit-learn:master Aug 16, 2017

thechargedneutron deleted the Secondary branch October 17, 2017 02:28

absolutelyNoWarranty mentioned this pull request Dec 15, 2017

[MRG+2] Add common test for set_params behavior #7760

Merged

		return isinstance(expected, type) and issubclass(expected, basetype)


		class _AssertRaisesContext(_AssertRaisesBaseContext):

		"assert_approx_equal", "SkipTest"]


		def _is_subtype(expected, basetype):

Uh oh!

[MRG] Backports msg in assert_raises and assert_raises_regex #9536

[MRG] Backports msg in assert_raises and assert_raises_regex #9536

Uh oh!

Conversation

thechargedneutron commented Aug 12, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

thechargedneutron commented Aug 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thechargedneutron commented Aug 13, 2017

Uh oh!

thechargedneutron commented Aug 13, 2017

Uh oh!

jnothman commented Aug 13, 2017 via email

Uh oh!

thechargedneutron commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Aug 15, 2017

Uh oh!

thechargedneutron commented Aug 15, 2017

Uh oh!

thechargedneutron commented Aug 15, 2017

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Aug 15, 2017

Uh oh!

thechargedneutron commented Aug 16, 2017

Uh oh!

jnothman commented Aug 16, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thechargedneutron commented Aug 16, 2017

Uh oh!

amueller commented Aug 16, 2017

Uh oh!

jnothman commented Aug 16, 2017

Uh oh!

thechargedneutron commented Aug 17, 2017

Uh oh!

jnothman commented Aug 17, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants