Skip to content
This repository was archived by the owner on Oct 14, 2020. It is now read-only.

@Test.timeout decorator in python doesn't behave correctly anymore #698

Closed
Blind4Basics opened this issue Feb 14, 2019 · 10 comments
Closed

Comments

@Blind4Basics
Copy link

Blind4Basics commented Feb 14, 2019

Describe the bug

Known fact: the decorator was tricky to use, but I had found a way and documented it in the wiki about the test framework.

  • known problem: if the call to the function of the user was done inside the decorator, he could raise an issue to stop the tests, then the decorator was swallowing the exception (thrown in another thread than the one of the test suite, so didn't trigger the failure of the test suite itself, iirc).
  • previous solution: like I documented it, using an intermediate function so that the call to the function of the user is in the current thread all the same.
  • current problem: that doesn't work anymore...:

To Reproduce

See this fork of anter69's solution to my "Ulam sequence" kata. I tried different things but only get the printing of the stack trace, the test suite is never crashing.
(edit: btw, there is something weird about the performances: I couldn't make my own solution to pass unless I lowered by 25% the load of the perf tests... x/. Tho that was valide before... That's unrelated to the current problem, tho. Just a note...)

Expected Behavior

Any exception thrown by the user or his code should interrupt the test with an actual failure.

Screenshot

image

@kazk
Copy link
Member

kazk commented Feb 14, 2019

I don't understand the "anymore" part. I haven't changed anything.

@Bubbler-4 any idea what's going on?

@Bubbler-4
Copy link
Contributor

Bubbler-4 commented Feb 14, 2019

No idea either. I didn't know about throwing errors in threads when I wrote it.

On the other hand, I found this SO answer to reraise child's exceptions in the parent. We can start working from there, though it sounds like a lot of work. Or, we can study and use pathos which is mentioned in a comment there.

UPDATE: I think this can be a workaround: That's not the right thing, the framework should actually have test.expect_no_error implemented.

@test.timeout(1.0)
def test_one():
    Test.expect_error('should not throw error', lambda: sol(some_args), ())
    Test.assert_equals(sol(some_args), 1)

So the expect_no_error should be

def expect_no_error(s, f, err = BaseException):
    try: f()
    except err as e: test.fail(s)

and the desired test case would be (also in this kumite fork)

test.expect_no_error(
    'Should not raise exceptions',
    lambda: test.assert_equals(ulam_sequence(u0, u1, n), ulamRef(u0, u1, n))
)

@Blind4Basics
Copy link
Author

Blind4Basics commented Feb 14, 2019

errr... "solitude" just posted that code as a fork. Is that actually one of you? x)

ok, that effectively works, even if it's absolutely unusable for the average user, I'd say... :/

Would it be possible to make that internal to the timeout decorator in some way? I fear not, because this hack is still subject to the original problem. For instance:

This one is catching correctly the exception raised in the code:

def doTests(nTests, n):
    a = range(1,100)
    for i in range(nTests):
        ...
        expect_no_error(
            'Should not raise exceptions',
            lambda: test.assert_equals(ulam_sequence(u0, u1, n), expected)
        )
        
caller = lambda: doTests(20, 1500)

@Test.describe('Random tests')
def rnd():
    
    @Test.it('Small sequences')
    def smalls(): doTests(20, rand(10,31))
    
    @Test.it('Large sequences')
    def bigs(): 
        @Test.timeout(11)
        def withTimer(): caller()

but this one doesn't anymore:

def doTests(nTests, n):
    a = range(1,100)
    for i in range(nTests):
        ...
        expect_no_error(
            'Should not raise exceptions',
            lambda: test.assert_equals(ulam_sequence(u0, u1, n), expected)
        )
        
#caller = lambda: doTests(20, 1500)

@Test.describe('Random tests')
def rnd():
    
    @Test.it('Small sequences')
    def smalls(): doTests(20, rand(10,31))
    
    @Test.it('Large sequences')
    def bigs(): 
        @Test.timeout(11)
        def withTimer(): doTest(20,1500)      #<<<<<

@Bubbler-4
Copy link
Contributor

You made a typo in the second code block (doTest should be doTests), and it indeed works as it should (doTests is already in our scope).

And... yes, solitude is my Haskellish self, though it's invading many other areas these days. Sorry if you feel bad about it.

@Blind4Basics
Copy link
Author

Blind4Basics commented Feb 14, 2019

x/

what the hell... Sorry... :/

edit: no, don't worry, I have no problem about your duplicity. ;)

@Bubbler-4
Copy link
Contributor

Bubbler-4 commented Feb 14, 2019

In order to avoid the hacks, I think this could work (given that we already have expect_no_error):

def timeout(limit):
    ...
    def wrapper(func):
        ...
        def child_process():
            expect_no_error('Should not raise exceptions', func)
        # create a child process with `child_process` as main
        # run and terminate after `limit` seconds

This is expected to work even without the external wrappers like doTests, since it catches all possible exceptions and prints a failure right away inside the child process. See the withTimer at the bottom in this kumite.

Thinking about the use cases involving exceptions, you don't usually want to raise specific exceptions when you write a performance kata, and for GeneratorExit, you normally handle it by simple iterable protocol. So catching BaseException so nothing can leak would be the way to go here.

@kazk Can I submit a PR on this repo, or are you planning to create a repo per language (for the ones with custom extensions, as in hspec-codewars)?

@kazk
Copy link
Member

kazk commented Feb 14, 2019

@Bubbler-4 Created https://github.com/Codewars/python-test-framework with current cw-2.py so you can fork it and open PR with changes.

@Bubbler-4
Copy link
Contributor

PR submitted. Anyone interested can see the demo kumite.

kazk pushed a commit to codewars/python-test-framework that referenced this issue Feb 15, 2019
* Add `expect_no_error(message, function, exception=BaseException)`

Tests if the function, when run, does not raise any exception that can be caught by `exception`. Generates a failed assertion if not met.

* Improve `@timeout(sec)`

The wrapped function is again wrapped inside `expect_no_error`, so that any exception raised inside the child process can be caught right there. If this happens, generates a failed assertion.

* Fix formatting

Fixes codewars/codewars-runner-cli#698
@kazk
Copy link
Member

kazk commented Feb 15, 2019

I'll let you guys know when this is deployed.

@kazk kazk reopened this Feb 15, 2019
@kazk kazk added language/python status/ready Ready to be deployed and removed status/ready Ready to be deployed labels Feb 15, 2019
@kazk
Copy link
Member

kazk commented Feb 16, 2019

Deployed. Thanks @Bubbler-4 for the fix.

@kazk kazk closed this as completed Feb 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants