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

Problems in the timeout utility #20

Open
Blind4Basics opened this issue Jun 19, 2021 · 4 comments
Open

Problems in the timeout utility #20

Blind4Basics opened this issue Jun 19, 2021 · 4 comments

Comments

@Blind4Basics
Copy link
Contributor

Blind4Basics commented Jun 19, 2021

ok, there are actually several problems about it.

Context

So far, it handles properly:

  • error raised in the function of the user while running
  • wrong output from the user
  • execution time too long
  • max buffer limit error

But it has two problems in its current state:

  1. tests are passing if the process just crashes because it runs out of memory
  2. The "nesting status" (describe/it) of the assertions done inside the timeout utility is ambiguous for one, and even worse, may be different depending on what the function passed by the user to the decorator is doing.

Developping point 1:

Used that way:

# differnet behaviours for the user function

def bufferLimitError():
    while 1: print('qkjdhgkjqhfgdg')
def slowAF():
    while 1: pass
def raiseError():  raise KeyError()
def testFails():   test.fail("")
def testPass():    test.pass_()
def floodRAM():    elemental_forms(random_evil_string(15000))    # this explodes the heap

BEHAVIOURS = (slowAF, raiseError, testFails, testPass, floodRAM, bufferLimitError)

@test.describe(f'timeout inside it blocks')
def tests():
    for i,behave in enumerate(BEHAVIOURS):
        @test.it(behave.__name__)
        def _():
            test.timeout(.1 if i!=4 else 8)(behave)

leads to floodRAM displaying no assertion result at all, meaning this is a "win" for the user if all other tests are passing.

image


Developping point 2:

Current state of the decorator:

def timeout(sec):
    def wrapper(func):
        from multiprocessing import Process
        msg = 'Should not throw any exceptions inside timeout'

        def wrapped():
            expect_no_error(msg, func)
            """ >>> func is supposed to do some assertions on the result of the user's function <<< """

        process = Process(target=wrapped)
        process.start()
        process.join(sec)
        if process.is_alive():
            fail('Exceeded time limit of {:.3f} seconds'.format(sec))
            """ >>> This is the problematic assertion <<< """
            process.terminate()
            process.join()
    return wrapper

So, imagining the following possible uses:

@test.describe(f'developping point 2')
def tests():
    @test.it('inside it block')
    def _():
        @test.timeout(1)
        def _():
            test.expect(True)
            
    @test.describe('wrapping describe/it blocks')
    def _():
        
        @test.timeout(1)
        def _():
            @test.it('lvl 1')
            def _():
                test.expect(True)
                
        @test.timeout(1)
        def _():
            @test.describe('meh')
            def _():
                @test.it('lvl 2')
                def _():
                    test.expect(True)

This leads to assertions that can be done outside of a @test.it block, which is what we are currently fighting against...
For now, the documentation tells nothing about that (about "how to use or not the timeout decorator", I mean)

image


Suggestions

Resolving point 1

I already found a way to handle this. I still have to test the behaviour in different situations to check that it works as expected.
The idea is to pass a Value to the Process, so that this value is updated only if the decorated function doesn't raise/crash. This value is examined afterward at the end of the wrapper to check that the process actually updated it

Resolving point 2

Here I'd need some guidance... I see mostly 3 ways out of this:

  1. State in the documentation that the timeout utility has to be used only inside a @test.it block. This way, nothing to change in the code about that.
  2. State in the documentation that the timeout utility has to be used wrapping the test.it block(s). Then we can add a new it block at the end of the wrapper to get the correct level.
  3. the flexible way... but might lead to more troubles than anything: add a new argument to the decorator stating if the final assertions of the wrapper must be made insde a it block or not. I sort of like the idea, and dislike it at the same time... :/

What would you prefere?

Corollary:

I blieve that the tests checking the framework (python-test-framework/tests/fixtures/) need an update too, to test for all those scenarii (except maybe the buffer limit error?)

final question?

That may be the occasion to add the option asked by Voile (see #4). Even if it' effectively doesn't seem that useful (see FArekkusu's answer), I like the idea of giiving more flexibility to the user.
Go or no go?

@Blind4Basics
Copy link
Contributor Author

ok, edited. You can assign me to it.

@Blind4Basics
Copy link
Contributor Author

Blind4Basics commented Jun 19, 2021

About "point 1":

I'm archiving this here, so that you can already take a look, because I have to go to sleep, soon.

I tested this on CW, but it should look like this in the file of the framework (not tested yet in this context)

def timeout(sec):
    def wrapper(func):
        from multiprocessing import Process, Value
        msg = 'Should not throw any exceptions inside timeout'

        def wrapped(finished):
            try:
                func()
                finished.value = 1.0
            except BaseException as e:
                finished.value = 1.0
                fail("{}: {}".format(msg or "Unexpected exception", repr(e)))
                
        finished = Value('d',0.0)
        # needed to know if the process crashed without any "feedback" and before any
        # assertion has been done in the wrapped function or the wrapper (happens if 
        # the heap memory explodes)
        
        process = Process(target=wrapped, args=(finished,))
        process.start()
        process.join(sec)
        
        if process.is_alive():
            fail('Exceeded time limit of {:.3f} seconds'.format(sec))
            process.terminate()
            process.join()
        elif not finished.value:
            fail('Something went wrong: the process running your function crashed without raising errors')
    return wrapper

I'm actually wondering about the message in the last fail. Any ideas/feedback about that? (or anything else)

This is working so far:

image

@Blind4Basics
Copy link
Contributor Author

oops, I closed it by mistake...

@Blind4Basics Blind4Basics reopened this Jun 20, 2021
@Blind4Basics
Copy link
Contributor Author

Blind4Basics commented Jun 28, 2021

I'm playing with the idea of allowing to wrap describe/it blocks with the timeout utility, this morning. Very bad idea, so far: I cannot get consistent behaviours. Surely I messed up my logic, but I cannot find where. Dropping the idea, for now.

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

No branches or pull requests

1 participant