tests(agent): Add tests that specifically exercise JIT.

[zsistla]

Convert forked PR branch to main repo branch so the ci/cd will run...

Please see original forked PR here: #583

[codecov-commenter]

Codecov Report

Merging #715 (47469df) into oapi (2b9e3b4) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             oapi     #715      +/-   ##
==========================================
+ Coverage   78.59%   78.65%   +0.05%     
==========================================
  Files         191      191              
  Lines       26629    26628       -1     
==========================================
+ Hits        20930    20944      +14     
+ Misses       5699     5684      -15     

Flag	Coverage Δ
agent-for-php-7.0	77.37% <ø> (-0.01%)	⬇️
agent-for-php-7.1	77.09% <ø> (-0.01%)	⬇️
agent-for-php-7.2	77.60% <ø> (-0.01%)	⬇️
agent-for-php-7.3	77.62% <ø> (-0.01%)	⬇️
agent-for-php-7.4	77.36% <ø> (-0.01%)	⬇️
agent-for-php-8.0	77.23% <ø> (+0.06%)	⬆️
agent-for-php-8.1	77.18% <ø> (+0.06%)	⬆️
agent-for-php-8.2	76.78% <ø> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[lavarou]

Why was this removed? The 'baseline' for this test has this:

newrelic-php-agent/tests/integration/span_events/test_span_events_are_created_upon_caught_exception.php

Lines 26 to 28 in acc7942

	/*EXPECT_ERROR_EVENTS
	null
	*/

[zsistla]

The original tests, which provided the prototype for this test, pass; however, these are not exact clones or they would be duplicate tests.

These tests have differences, namely:
https://github.com/newrelic/newrelic-php-agent/pull/715/files/acc7942cb672999fd87537be9aae508915bcc916#diff-61ae0ae768b1f2332109c0aad270b311e54933445589dc3249ce023b009b78eaR24-R26

So we are using PHP's native error handling in a slightly different way.
Because of the differences and how PHP handles errors, for PHP 8.0/8.1 it creates an error when it handles the uncaught exception and the agent create an error event with that error. However, PHP 8.2 handles an uncaught exception slightly differently and doesn't generate an error when it handles an uncaught exception.

[lavarou]

The original tests, which provided the prototype for this test, pass; however, these are not exact clones or they would be duplicate tests.

Yes, they are not duplicates. The modify runtime execution of original tests, by:

1. loading opcache module
2. enabling opcache
3. enabling JIT

However, the agent's behavior should persist regardless of this change in runtime. Yet, agent's behavior changes, and this code block:

/*EXPECT_ERROR_EVENTS
null
*/

exposes those behavior differences. I.e. ERROR_EVENTS are generated in PHPs 8.0 and 8.1 but are not generated in PHP 8.2.

[lavarou]

If the behavior of the agent is supposed to be different depending on PHP version, then this test should be PHP version specific.

[zsistla]

This behavior differs between PHP 8.0/8.1 and PHP 8.2 because PHP used to accept less strongly typed error handlers without explicit parameters. Starting with PHP 8.2 WHEN opcache is enabled
, error handlers need to have explicit parameters.
As such, exceptions handled by old style error handlers will be correctly propagated for PHP 8.0/8.1,
but PHP 8.2 and above requires strongly typed error handlers otherwise the object it returns isn't explicitly recognizable as a Throwable.

The tests are split to accommodate the difference.

Please see: d139ac6 which does the following:

• Split tests since PHP used to accept less strongly typed error handlers without explicit parameters.
  Starting with PHP 8.2, error handlers need to have explicit parameters.
  As such, exceptions handled by old style error handlers will be correctly propagated for PHP 8.0/8.1,
  but PHP 8.2 and above requires strongly typed error handlers otherwise the object it returns isn't explicitly recognizable as a Throwable.
  To cover both cases, there is now one test for PHP 8.0/8.1 with old style error handler and one test for PHP 8.0+ with new style handler.
• Updated description to explicitly determine if error events are created since previous description only noted if span events were created.
• Updated names to more accurately reflect test behavior.

[lavarou]

I don't think 38eb42d and d139ac6 is the right approach here. The crux of the issue is that nr_php_error_zval_is_exception in PHP 8.2 when opcache is enabled is not able to confirm that the zval it got is of Throwable type. According to https://www.php.net/manual/en/function.set-exception-handler.php#refsect1-function.set-exception-handler-parameters, the $callback should be a function that accepts exactly one parameter, which will be the Throwable object. The handler in the test is a closure that doesn’t expect and/or use any parameters. The incorrect PHP code leads to undefined behavior of the agent, which is what this issue is all about. To address this issue, the test code needs to be fixed in the first place. When the test code uses valid PHP code, the OAPI agent behaves the same for all PHP versions regardless if opcache is enabled or disabled and therefore the split between PHP versions is not needed. This PR #748 reverts 38eb42d and d139ac6 and fixes the PHP code in tests.

However, #748 will expose a different issue: when DT is enabled, OAPI agent generates error_events and pre-OAPI agent does not generate error_events (tests/integration/span_events/test_span_events_are_created_upon_caught_exception.php fails for OAPI agent because it has this expectation). This behavior difference is caused by this code difference:

• pre-oapi agent checks if nr_txn_should_create_span_events(NRPRG(txn)) here and generates error_event only when current request will not cause spans to be created
• oapi agent does not check if current request will or will not cause spans to be created here and and generates error_event unconditionally.

To address this issue we need to answer this question: "Which behavior is correct: pre-OAPI agent or OAPI agent?"

[lavarou]

After further discussion it was determined that:

• this PR changes tests in tests/integratio/jit subtree only, and therefore the signature of user exception handler in tests/integration/span_events/test_span_events_are_created_upon_caught_exception.php is not going to be fixed by this PR. tests: fix user exception and error handler implementation #748 will be re-purposed to address fixing invalid test code in tests outside tests/integratio/jit subtree.
• it is ok to leave test_span_events_are_created_upon_uncaught_exception.php -> test_span_events_are_created_upon_uncaught_unhandled_exception.php and test_span_events_are_created_upon_caught_exception.php -> {test_span_events_are_created_upon_uncaught_handled_exception.php and test_span_events_are_created_upon_uncaught_handled_exception_invalid_handler.php} file renames in this PR
• the behavior in oapi agent of generating error events unconditionally is the correct behavior; either re-purposed tests: fix user exception and error handler implementation #748 or new PR will address this behavior change and update/split test_span_events_are_created_upon_caught_exception.php and/or other tests that have this issue depending on PHP version.

[lavarou]

I have a few 'cosmetic' updates which, hopefully, improve test descriptions as well as test expectations.

[ZNeumann]

Great work and very thorough conversations!