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

Register shutdown function and throw exception if the file was not intercepted #13

Closed
wants to merge 1 commit into from

Conversation

maks-rafalko
Copy link
Member

@maks-rafalko maks-rafalko commented Jun 21, 2020

Register shutdown function and throw exception if the file was not intercepted in the current process by Infection's stream wrapper

see infection/infection#1275

When the project uses https://github.com/dg/bypass-finals, this lib unregisters Infection's stream wrapper and registers its own. It leads to Mutant (mutated file) not being used instead of original one.

Thus, all mutants are escaped.

With this update, Infection's user will see an error in the Infection's log with a clear message:

Before:

.: killed, M: escaped, U: uncovered, E: fatal error, T: timed out

.....                                                (5 / 5)

After:

Processing source code files: 2/2
.: killed, M: escaped, U: uncovered, E: fatal error, T: timed out

EEEEE                                                (5 / 5)

And part of the log file:

3) /infection/tests/e2e/Stream_Wrapper_Execution/src/FinalClass.php:11    [M] OneZeroInteger

--- Original
+++ New
@@ @@
 {
     public function get() : int
     {
-        return 1;
+        return 0;
     }
 }

  PHPUnit 8.5.7 by Sebastian Bergmann and contributors.

  ..                                                                  2 / 2 (100%)

  Time: 53 ms, Memory: 6.00 MB

  OK (2 tests, 3 assertions)

  Fatal error: Uncaught LogicException: Infection's IncludeInterceptor was not executed. Make sure you don't use any `file://` Stream Wrappers (like dg/bypass-finals) in /infection/include-interceptor/src/IncludeInterceptor.php:90
  Stack trace:
  #0 [internal function]: Infection\StreamWrapper\IncludeInterceptor::Infection\StreamWrapper\{closure}()
  #1 {main}
    thrown in /infection/include-interceptor/src/IncludeInterceptor.php on line 90

…tercepted in the current process by Infection's stream wrapper

see infection/infection#1273
'Infection\'s IncludeInterceptor was not executed. Make sure you don\'t use any `file://` Stream Wrappers (like dg/bypass-finals)'
);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

  1. How about a dedicated function that we will call from our side? Like ::setup(?callable $shutdownHandler)
  2. Isn't there a problem if a user happen to call ::intercept() multiple times in a row?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about a dedicated function that we will call from our side? Like ::setup(?callable $shutdownHandler)

do you mean in addition to current code, or instead? Could you please explain how we will use it?

What I wanted to achieve is

  1. to not modify any code where Interceptor::intercept() is used
  2. to make the default shutdown handler
  3. to make it possible to override default shutdown handler to unit test the class. With the default shutdown handler - it's impossible to run unit tests without exit code equal to 0 - it always fails

So, example with ::setup() will be useful for understanding.

Isn't there a problem if a user happen to call ::intercept() multiple times in a row?

Nothing bad. The first handler is executed, throws an exception, and the second one is not executed. So I would say in the current state it's safe to call ::intercept() 2 times. And I guess this will never be the case :)

But if you think we need to avoid it, I can wrap register_shutdown_handler() with if and do it only once regardless of how many times ::intercept() is called

Copy link
Member Author

Choose a reason for hiding this comment

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

ping @sanmai

Copy link
Member

@sanmai sanmai Jun 28, 2020

Choose a reason for hiding this comment

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

Right. I think best way to tackle the issue would be to avoid callbacks and register_shutdown_handler() altogether. IncludeInterceptor's job is to make intercept happen. It is not their job to control for this. That's one argument. Another reason to avoid register_shutdown_handler() is that it is difficult to catch exceptions thrown from there.

So I think $wasIntercepted is fine and good, only missing here is an accessor to read it. If we'd have a method ::wasIntercepted(), we can call it, say, from here, and then we'll be free to do whatever we want, be it an exception, or a different exit code. (I hope there is a way, I just don't see it atm)

Copy link
Member Author

Choose a reason for hiding this comment

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

and then we'll be free to do whatever we want, be it an exception, or a different exit code.

but where we can do it? Mentioned code becomes an autoloader.php for test framework. And this is the only one place where Infection has custom code for executed test suite.

And register_shutdown_handler() was a workaround to add "after test framework execution" event and do some checks there.

So, without using register_shutdown_handler(), how would you accomplish it?

Copy link
Member

Choose a reason for hiding this comment

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

Al very least we can use this register_shutdown_handler() inside mentioned autoloader.php, but let me see if there's a better way.

@maks-rafalko
Copy link
Member Author

Closing in favor of dg/bypass-finals#9 (comment)

@maks-rafalko maks-rafalko deleted the bugfix/register-shutdown branch September 10, 2022 11:24
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.

None yet

2 participants