Skip to content

Test oci8 & pdo_oci in CI #8348

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 1 commit into from
Closed

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Apr 12, 2022

close #8346

@mvorisek mvorisek changed the base branch from master to PHP-8.0 April 12, 2022 11:21
@cmb69 cmb69 linked an issue Apr 12, 2022 that may be closed by this pull request
@mvorisek mvorisek force-pushed the ci_test_oracle branch 4 times, most recently from fc0926c to 7550cd7 Compare April 17, 2022 12:07
@mvorisek mvorisek marked this pull request as ready for review April 17, 2022 12:07
@mvorisek mvorisek force-pushed the ci_test_oracle branch 2 times, most recently from 542f6b7 to 378291f Compare April 17, 2022 19:34
@mvorisek
Copy link
Contributor Author

@cmb69 any idea how to fix Warning: zend_signal: handler was replaced for signal (2) after startup in Unknown on line 0 issue?

@cjbj
Copy link
Contributor

cjbj commented Apr 18, 2022

I expect you have a debug PHP build. That warning goes away with a non-debug build. One thing I don't recall whether I tried was to add DIAG_SIGHANDLER_ENABLED=FALSE to a sqlnet.ora file on the PHP host. I'm sorry I'll be out and won't get a chance to try this for a while, but I'll ask a colleague to test.

@mvorisek
Copy link
Contributor Author

mvorisek commented May 9, 2022

@cjbj is it possible to disable the signal override completely from the C code (/wo any special env. var)?

@mvorisek
Copy link
Contributor Author

@cjbj any idea how to disable the signal override from the oci8 ext itself?

@cjbj
Copy link
Contributor

cjbj commented May 26, 2022

@mvorisek apologies I've been slammed with this Python driver release and haven't been able to play with PHP. As well as the previous suggestion, it may be possible to use DISABLE_INTERRUPT=on in a sqlnet.ora (yes, this is not particularly documented - the Net team tell me it will be). I don't believe there is a way in the connect string to do this, and nothing in an API call that OCI8 could make.

@mvorisek mvorisek force-pushed the ci_test_oracle branch 2 times, most recently from f2cc01c to 03a5e4d Compare May 26, 2022 19:35
@mvorisek mvorisek marked this pull request as draft May 26, 2022 20:09
@mvorisek
Copy link
Contributor Author

@cjbj thanks, the signal override issues is gone!

these tests are failing:

the first two tests are probably not compatible /w the latest Oracle DB

the last one is probably an issue in php

can you please take a look and advise how to fix them?

@cmb69
Copy link
Member

cmb69 commented May 27, 2022

That looks like an issue in oci_stmt_param_hook() where PDO::PARAM_NULL is not explicitly handled, contrary to other drivers.

PS: somewhat related issue regarding PDO::PARAM_BOOL: https://bugs.php.net/81586

@mvorisek
Copy link
Contributor Author

in PHP 8.1 there are two tests failing - https://github.com/mvorisek/php-src/runs/7404701507

  • ext/pdo_oci/tests/pdo_039.phpt is easy to fix - mvorisek@2888cb5
  • ext/pdo_oci/tests/pdo_oci_quote1.phpt - please help me how to fix it, it seems null value is supported by PDO->quote() by all other DB vendors

@cmb69
Copy link
Member

cmb69 commented Jul 19, 2022

That looks right.

  • ext/pdo_oci/tests/pdo_oci_quote1.phpt - please help me how to fix it, it seems null value is supported by PDO->quote() by all other DB vendors

PDO:::quote() expects a string as first argument; passing null to non-nullable parameters is deprecated as of PHP 8.1. Since null is converted to an empty string, and there is also a test for an empty string, I suggest to just remove that null check:

 ext/pdo_oci/tests/pdo_oci_quote1.phpt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/pdo_oci/tests/pdo_oci_quote1.phpt b/ext/pdo_oci/tests/pdo_oci_quote1.phpt
index 024625a181..68df2ea926 100644
--- a/ext/pdo_oci/tests/pdo_oci_quote1.phpt
+++ b/ext/pdo_oci/tests/pdo_oci_quote1.phpt
@@ -21,7 +21,7 @@ $stmt = $db->prepare('select * from poq_tab');
 // The intent is that the fetched data be identical to the unquoted string.
 // Remember!: use bind variables instead of PDO->quote()
 
-$a = array(null, "", "a", "ab", "abc", "ab'cd", "a\b\n", "'", "''", "a'", "'z", "a''b", '"');
+$a = array("", "a", "ab", "abc", "ab'cd", "a\b\n", "'", "''", "a'", "'z", "a''b", '"');
 foreach ($a as $u) {
     $q = $db->quote($u);
     echo "Unquoted : ";

And adjust the expectations accordingly.

Can you please provide a separate PR for these fixes?

@mvorisek
Copy link
Contributor Author

I fixed the tests from PHP 8.0 up to master. Please merge (in order):

and then this PR, tests should pass in all PHP branches

@mvorisek mvorisek marked this pull request as draft August 22, 2022 09:15
@mvorisek mvorisek marked this pull request as ready for review August 22, 2022 09:31
@mvorisek
Copy link
Contributor Author

This PR is done, fixed into PHP 8.0 and PHP 8.1 were merged, so tests should pass up too a master. Please merge into PHP 8.0.

@mvorisek mvorisek deleted the ci_test_oracle branch August 24, 2022 14:50
@mvorisek
Copy link
Contributor Author

@cmb69 it seems CI fails randomly - see https://github.com/php/php-src/runs/7998206010?check_suite_focus=true#step:10:108 - do you have any idea how to debug this? Does all table names needs to be unique across all Oracle tests? Or can the tests all be marked somehow to be not run in parallel?

@kocsismate
Copy link
Member

According to https://stackoverflow.com/a/4842778/1692726, this may happen because:

Your table is already locked by some query. For example, you may have executed "select for update" and have not yet committed/rollbacked and fired another select query. Do a commit/rollback before executing your query.

So yes, I guess the easiest way to quick fix the issue for now is to add a --CONFLICTS-- section. I'm not sure based on the SO answer if it would work as a long term solution if all these statements were executed in a transaction which were then discarded. But otherwise, using different tables for these tests also seems like a good idea.

@cmb69
Copy link
Member

cmb69 commented Aug 25, 2022

If (almost) all tests could be conflicting, instead of a --CONFLICTS-- section for each file, there could be a single file named CONFLICTS in the tests directory.

mvorisek added a commit to mvorisek/php-src that referenced this pull request Aug 26, 2022
kocsismate pushed a commit that referenced this pull request Aug 26, 2022
kocsismate added a commit that referenced this pull request Aug 26, 2022
* PHP-8.0:
  Fix GH-8348 for nightly
kocsismate added a commit that referenced this pull request Aug 26, 2022
* PHP-8.0:
  Fix GH-8348 for nightly
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Sep 7, 2022
@kocsismate
Copy link
Member

@mvorisek The OCI tests are very flaky. Could you please take a look at them? E.g: https://github.com/php/php-src/runs/8290051982?check_suite_focus=true#step:11:112

@mvorisek
Copy link
Contributor Author

@kocsismate yes, please see #9430 (comment). It seems the ext/oci8/tests/extauth_*.phpt tests are somehow badly designed. I do not use the tested functions, so any help appreciated.

@cmb69
Copy link
Member

cmb69 commented Sep 11, 2022

I'm afraid that none of the core contributors uses oci8 at all, so this turns out to be difficult. Isn't there a possibility to set some connection timeout?

@mvorisek
Copy link
Contributor Author

IDK, probably not or the tests will require a big modifications, as the only params are accepted thru the DSN string in oci_connect. Then basic DSN string tests will be useless...

I do not use oci8 either, I have contributed this CI testing and we need robust support for our atk4/data PHP framework, where we want to support all major DB vendors.

I understand the importance to keep the CI green and will try to debug these tests locally /w Oracle docker DB.

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.

oci8 / pdo_oci exts are not tested in CI
5 participants