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

unit tests failing because app.close calls sys.exit #313

Closed
vroomfondle opened this Issue Jun 2, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@vroomfondle

I just upgraded from 2.4 -> 2.6 and my unit tests started failing because I'm calling CementApp.close() (as is done in the example at http://builtoncement.com/2.6/dev/testing.html), and that in turn appears to be calling sys.exit(). The tests don't fail when using 2.4.

I can't find any mention of this change of behaviour in the documentation, and I can't see a warning in the change log (sorry if I'm just being blind). Was it an intentional change? Personally I don't think it's a good idea for a framework to call sys.exit() unless explicitly told to do so, because the bootstrap code might want to do other stuff after the application is closed (in this case, it wants to continue running tests). Is there a way to prevent it, or to achieve the same effect as close() without the sys.exit()? If so, perhaps a description of how to do this could be added to the documentation?

Traceback (most recent call last):
File "/opt/sds/bin/admin/app/test/test_calculate_ects_grades_controller.py", line 41, in tearDown
self.app.close()
File "/opt/sds/bin/admin/lib/python2.6/site-packages/cement/core/foundation.py", line 804, in close
sys.exit(self.exit_code)
SystemExit: 0

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 2, 2015

Member

I just ran into this issue myself after upgrading another app to Cement 2.6, and my immediately thought was "Eww.... yeah, probably should't have implemented sys.exit()". I think this change will be reverted and be more explicit, perhaps like needing to call 'app.exit()'.

I worked around this by sub-classing my app and creating a separate app for testing that catches SystemExit and just passes:

class MyTestApp(MyApp):
    def close(self, *args, **kw):
        try:
            super(MyTestApp, self).close(*args, **kw)
        except SystemExit as e:
            # ignore SystemExit from app.close() for tests
            pass

In my tests I do something like:

def test_myapp(self):
        app = MyTestApp(argv=['some-command', '--foo=bar'])
        with app:
            app.run()

Will keep this issue and slate it for Cement 2.8 to remove the implicit call to sys.exit().

Member

derks commented Jun 2, 2015

I just ran into this issue myself after upgrading another app to Cement 2.6, and my immediately thought was "Eww.... yeah, probably should't have implemented sys.exit()". I think this change will be reverted and be more explicit, perhaps like needing to call 'app.exit()'.

I worked around this by sub-classing my app and creating a separate app for testing that catches SystemExit and just passes:

class MyTestApp(MyApp):
    def close(self, *args, **kw):
        try:
            super(MyTestApp, self).close(*args, **kw)
        except SystemExit as e:
            # ignore SystemExit from app.close() for tests
            pass

In my tests I do something like:

def test_myapp(self):
        app = MyTestApp(argv=['some-command', '--foo=bar'])
        with app:
            app.run()

Will keep this issue and slate it for Cement 2.8 to remove the implicit call to sys.exit().

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 2, 2015

Member

Sorry, I just looked at the code for CementApp and realized/remembered that I did build in a work-around for this:

exit_on_close = True
"""
Whether or not to call ``sys.exit()`` when ``close()`` is called.
Generally only used for testing to avoid having to catch
``SystemExit`` three thousand times.

So, you could disable this feature by:

class MyApp(CementApp):
    class Meta:
        exit_on_close = False

Alternatively, if you want to keep it but just disable it for testing you could follow the above example of sub-classing to a test app and just overriding the exit_on_close meta-data option there.

Hope that helps!

Member

derks commented Jun 2, 2015

Sorry, I just looked at the code for CementApp and realized/remembered that I did build in a work-around for this:

exit_on_close = True
"""
Whether or not to call ``sys.exit()`` when ``close()`` is called.
Generally only used for testing to avoid having to catch
``SystemExit`` three thousand times.

So, you could disable this feature by:

class MyApp(CementApp):
    class Meta:
        exit_on_close = False

Alternatively, if you want to keep it but just disable it for testing you could follow the above example of sub-classing to a test app and just overriding the exit_on_close meta-data option there.

Hope that helps!

@derks derks added this to the 2.7.2 Development milestone Jun 2, 2015

@derks derks self-assigned this Jun 2, 2015

@derks

This comment has been minimized.

Show comment
Hide comment
@derks

derks Jun 2, 2015

Member

Note: Should be documented in dev/testing documentation properly to avoid surprises. Alternatively, might consider making exit_on_close = False the default and adding the SystemExit to app.exit() rather than app.close().

Member

derks commented Jun 2, 2015

Note: Should be documented in dev/testing documentation properly to avoid surprises. Alternatively, might consider making exit_on_close = False the default and adding the SystemExit to app.exit() rather than app.close().

@vroomfondle

This comment has been minimized.

Show comment
Hide comment
@vroomfondle

vroomfondle Jun 3, 2015

Excellent, thanks, that works. Defaulting exit_on_close to False sounds like a good idea to me.

Excellent, thanks, that works. Defaulting exit_on_close to False sounds like a good idea to me.

derks added a commit that referenced this issue Jun 20, 2015

derks added a commit that referenced this issue Jun 20, 2015

@derks derks closed this Jun 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment