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

Adding Logic In Case Of Exit #4604

Merged
merged 2 commits into from Dec 5, 2017

Conversation

Projects
None yet
2 participants
@Fenikkusu
Contributor

Fenikkusu commented Nov 3, 2017

I have been working on getting my testing setup within a CI. I was looking at the output of one such run when I realized that the test did not appear to be finishing, but was not giving any errors. After digging into it, I learned that one of my Junior Developers created a new class following some of the legacy code in the system. They put includes inside a class file (rather than relying on the PSR-4 Autoloader). One of these includes was a script to check a user's login/session and redirecting them to the homepage if they weren't logged in. Because a call to exit without params is equivalent to exit(0), my CI system thought that the test completed succesfully when it didn't actual do so.

This change is an attempt to catch such instances in the future. I use a shutdown function defined specifically for the execute to throw an exception in case the shutdown occurs.

Please note that I've not studied the Codeception Codebase so there may be a better way to detect this situation.

@Fenikkusu

This comment has been minimized.

Show comment
Hide comment
@Fenikkusu

Fenikkusu Nov 3, 2017

Contributor

After reviewing the code a bit more, I moved the logic into SuiteManager::run instead of Run::execute. It made more sense and fixed the issues with testing.

Contributor

Fenikkusu commented Nov 3, 2017

After reviewing the code a bit more, I moved the logic into SuiteManager::run instead of Run::execute. It made more sense and fixed the issues with testing.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Nov 10, 2017

Member

Thanks for the contribution!
Sorry for taking that long to respond. But we already have the shutdown function, it's located in https://github.com/Codeception/Codeception/blob/2.3/src/Codeception/Subscriber/ErrorHandler.php

Could you update that class in order to keep the core classes decoupled from error listeners

Member

DavertMik commented Nov 10, 2017

Thanks for the contribution!
Sorry for taking that long to respond. But we already have the shutdown function, it's located in https://github.com/Codeception/Codeception/blob/2.3/src/Codeception/Subscriber/ErrorHandler.php

Could you update that class in order to keep the core classes decoupled from error listeners

@Fenikkusu

This comment has been minimized.

Show comment
Hide comment
@Fenikkusu

Fenikkusu Nov 29, 2017

Contributor

I've applied the updates. I did have to update another one of the tests that was creating an instance of the ErrorHandler and causing a Did Not Finish Error.

Contributor

Fenikkusu commented Nov 29, 2017

I've applied the updates. I did have to update another one of the tests that was creating an instance of the ErrorHandler and causing a Did Not Finish Error.

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Nov 29, 2017

Member

Looks ok to me!

Member

DavertMik commented Nov 29, 2017

Looks ok to me!

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Dec 5, 2017

Member

Thanks!

Member

DavertMik commented Dec 5, 2017

Thanks!

@DavertMik DavertMik merged commit fba4837 into Codeception:2.3 Dec 5, 2017

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
wercker/build Wercker pipeline passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment