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

Repeat JIT tests #12495

Closed
wants to merge 10 commits into from
Closed

Repeat JIT tests #12495

wants to merge 10 commits into from

Conversation

danog
Copy link
Contributor

@danog danog commented Oct 22, 2023

Based on #12406, should be merged once #12428 + #12494 are closed

@danog
Copy link
Contributor Author

danog commented Oct 22, 2023

btw @iluuu1994 #12406 can be merged independently of this :)

@danog danog changed the title Add infection to testsuite, repeat JIT tests Add infection to nightly testsuite, repeat JIT tests Oct 22, 2023
.github/actions/test-linux/action.yml Outdated Show resolved Hide resolved
@@ -43,6 +43,7 @@ runs:
export TEST_PHP_JUNIT=junit.out.xml
export STACK_LIMIT_DEFAULTS_CHECK=1
sapi/cli/php run-tests.php -P -q ${{ inputs.runTestsParameters }} \
${{ inputs.jitType == 'disable' && '' || '--repeat 2' }}
Copy link
Member

Choose a reason for hiding this comment

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

CircleCI already tests with --repeat 2, I think that's enough.

'branch' => $branch,
'debug' => true,
'zts' => true,
'configuration_parameters' => "CFLAGS='-fsanitize=undefined,address -DZEND_TRACK_ARENA_ALLOC' LDFLAGS='-fsanitize=undefined,address'",
'run_tests_parameters' => '--asan',
'test_function_jit' => false,
Copy link
Member

Choose a reason for hiding this comment

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

Note that function JIT was specifically disabled on ASAN because it exceeded the 6h limit on GitHub Actions.

@@ -43,6 +43,7 @@ runs:
export TEST_PHP_JUNIT=junit.out.xml
export STACK_LIMIT_DEFAULTS_CHECK=1
sapi/cli/php run-tests.php -P -q ${{ inputs.runTestsParameters }} \
${{ inputs.jitType == 'disable' && '' || '--repeat 2' }}
Copy link
Member

Choose a reason for hiding this comment

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

The issue with combining --repeat with ASAN is that --repeat disabled certain tests (e.g. all tests with a CLEAN section). So this would essentially stop testing all DB extensions on ASAN, amongst other things.

@@ -83,7 +83,7 @@ set OPENSSL_CONF=
rem set SSLEAY_CONF=

rem prepare for OPcache
if "%OPCACHE%" equ "1" set OPCACHE_OPTS=-d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M -d opcache.jit_max_root_traces=100000 -d opcache.jit_max_side_traces=100000 -d opcache.jit_max_exit_counters=100000 -d opcache.jit=tracing
if "%OPCACHE%" equ "1" set OPCACHE_OPTS=-d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M -d opcache.jit_max_root_traces=100000 -d opcache.jit_max_side_traces=100000 -d opcache.jit_max_exit_counters=100000 -d opcache.jit=tracing --repeat 2
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't think --repeat should be added unless we can make sure it works for all tests, or unless the architecture is already tested without --repeat.

echo memory_limit=-1 >> /etc/php.d/opcache.ini
php -v
- name: Test Infection
Copy link
Member

Choose a reason for hiding this comment

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

I think your PRs still do too much at a time 🙂 E.g. just adding Infection without jit_check.php or other changes would be much easier to review and merge.

@danog danog changed the title Add infection to nightly testsuite, repeat JIT tests Repeat JIT tests Feb 1, 2024
@danog danog closed this Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants