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

Improve error handling #71

Open
wants to merge 3 commits into
base: 2.0
Choose a base branch
from

Conversation

marcovtwout
Copy link

@marcovtwout marcovtwout commented May 19, 2021

Improve error handling in a number of ways (see commit messages).

If not all parts of this PR are desirable in general, feel free to cherry pick.

Also fixes #41 and #37

if (!headers_sent()) {
header('X-Codeception-CodeCoverage-Error: ' . str_replace("\n", ' ', $message), true, 500);
}
setcookie('CODECEPTION_CODECOVERAGE_ERROR', $message);

echo 'CodeCeption C3 Error: ' . $message;
exit(1);
Copy link
Member

@Naktibalda Naktibalda May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is actually a major change in behaviour.
Before, c3 set a cookie and website continued to work as usual, now it fails loud and clear and the website doesn't work. It isn't a problem for tests, but, if c3.php is included to live website, it would break website for real users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if-clause here should protect breaking any website when not running tests:

c3/c3.php

Lines 34 to 36 in f7811b9

if (!array_key_exists('HTTP_X_CODECEPTION_CODECOVERAGE', $_SERVER)) {
return;
}

Also note that any errors encountered could already "break" a website, since a 500 response code is set when no headers are sent yet:

c3/c3.php

Line 54 in f7811b9

header('X-Codeception-CodeCoverage-Error: ' . str_replace("\n", ' ', $message), true, 500);


I know this part changes behavior when running tests and is perhaps better suited for a new major version. There's actually two points to this change:

  • Echo error message. Possibly makes it easier to debug for first-time users. Showing details could be made optional.
  • Stop c3 collection when encountering error. Most calls to __c3_error() look like they should stop execution of c3, but do not. For example, when Codeception is not loaded an error is thrown (

    c3/c3.php

    Lines 72 to 74 in f7811b9

    } else {
    __c3_error('Codeception is not loaded. Please check that either PHAR or Composer package can be used');
    }
    ) but execution continues. This could lead to more errors or unexpected behavior. Another way to solve this is to make those calls throw proper exceptions and catch them to log error and continue normal application execution.

@marcovtwout
Copy link
Author

@Naktibalda Would this PR be acceptable for a new major version of c3?

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

Successfully merging this pull request may close these issues.

Could not write error to log file
2 participants