Skip to content

Fix tmp file deletion in mysqli_pam_sha256_public_key_ini.phpt test #8427

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

Closed
wants to merge 2 commits into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Apr 22, 2022

the 1st (revert) commit should be merged into PHP 8.0 branch

#8295 was designed to detect also residual/uncleaned tmp files from build/tests

it works with PHP 8.0 & master, but one test for PHP 8.1 has an issue:

image

image

for some reasons the mysqlnd.sha256_server_public_key="test_sha256_ini" config is not deleted by the CLEAN section

iluuu1994 and others added 2 commits April 22, 2022 18:37

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
This reverts commit 1dc6dba.
@mvorisek mvorisek force-pushed the assert_no_tmp_files branch from 7a28b68 to 0508def Compare April 22, 2022 20:40
@mvorisek
Copy link
Contributor Author

I located the issue, to fix it, do this:

  1. cheery pick 400c605 into PHP 8.1 branch
  2. revert 1dc6dba into PHP 8.0 branch
  3. merge up
  4. close this PR

@mvorisek mvorisek marked this pull request as ready for review April 22, 2022 21:23
mvorisek referenced this pull request Apr 22, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
@mvorisek
Copy link
Contributor Author

mvorisek commented May 4, 2022

@iluuu1994 can you please apply the fixes based on my last post here?

@iluuu1994
Copy link
Member

I'm not convinced this check is worthwhile. It isn't a security or stability issue (which was the real motivation), and any skipped test is not checked, meaning if we only execute this check for certain environments only a sub-set of tests will be checked for cleaning up files.

@mvorisek
Copy link
Contributor Author

mvorisek commented May 4, 2022

it asserts that tests does not produce any residual files (files not ignored by .gitignore), yes, we check for env. we run it on, but it improves test quality, so if there is no downside, I think it is legit, see #8427 (comment), commit from step one was supposed to be commit into PHP 8.1, not master only, and my change will detect that

@ramsey
Copy link
Member

ramsey commented May 22, 2022

I want to make sure I understand this...

  • We should cherry-pick cba5e60 into PHP-8.0
  • Merge up into PHP-8.1
  • Then cherry-pick 0508def into PHP-8.1

Do I have that right?

@mvorisek
Copy link
Contributor Author

@ramsey yes and thank you into getting into this

kamil-tekiela added a commit that referenced this pull request May 31, 2022

Verified

This commit was signed with the committer’s verified signature.
kamil-tekiela Kamil Tekiela
Backported from master. See #8427
@mvorisek
Copy link
Contributor Author

mvorisek commented Jul 8, 2022

I want to make sure I understand this...

* We should cherry-pick [cba5e60](https://github.com/php/php-src/commit/cba5e609e43b8ea3dc5b4403e8c542c43b9a378e) into PHP-8.0

* Merge up into PHP-8.1

* Then cherry-pick [0508def](https://github.com/php/php-src/commit/0508def9a9a00456dee0569539f95ff80d6accfa) into PHP-8.1

Do I have that right?

The last step was already done by @kamil-tekiela, thank you. Now let's please cherry-pick cba5e60 into PHP-8.0, so the CI asserts tests produce no residual files after they are run. The issue, and that's why it the commit has been reverted by @iluuu1994, was fixed.

Then please close this PR.

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Jul 11, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
Closes phpGH-8427
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Jul 11, 2022

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo
Closes phpGH-8427
@mvorisek
Copy link
Contributor Author

closing in favor of #8977, thank you @iluuu1994

@mvorisek mvorisek closed this Jul 11, 2022
@mvorisek mvorisek deleted the assert_no_tmp_files branch July 11, 2022 15:57
Girgias pushed a commit that referenced this pull request Jul 21, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Closes GH-8427
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.

None yet

4 participants