Skip oci8 tests when no database is available #11820
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most oci8 tests fail out-of-the-box because a typical host won't have an Oracle database instance available. Other database drivers like mysqli and pgsql address this problem with an include file, inserted into SKIPIF, that skips the test if no connection at all can be made. This commits adds such a file (skipifconnectfailure.inc) for oci8, and adds the corresponding SKIPIF to any tests that connect to a database. Closes phpGH-11804
|
I don't have an Oracle instance to test the "success" case against but it looks like the CI does, so that's nice. |
cjbj
reviewed
Jul 31, 2023
| oci_close($c); | ||
| } | ||
| else { | ||
| $msg = "skip Failed to connect to Oracle instance "; |
There was a problem hiding this comment.
Minor comment: Say "Oracle Database instance"
|
This can be merged as, but I would have ideally preferred to see it included in the skipif.inc file to make that file have all the generic skip test logic - it already includes connect.inc. However, let's merge this PR and move on. Thanks for submitting it!!! |
Change "Oracle instance" to "Oracle Database instance" in an error message.
|
No problem, thanks for reviewing it. I've updated that bit of wording. |
Girgias
approved these changes
Jul 31, 2023
jorgsowa
pushed a commit
to jorgsowa/php-src
that referenced
this pull request
Aug 16, 2023
Most oci8 tests fail out-of-the-box because a typical host won't have an Oracle database instance available. Other database drivers like mysqli and pgsql address this problem with an include file, inserted into SKIPIF, that skips the test if no connection at all can be made. This commits adds such a file (skipifconnectfailure.inc) for oci8, and adds the corresponding SKIPIF to any tests that connect to a database. Closes phpGH-11804 * ext/oci8/tests/lob_aliases.phpt: drop unnecessary SKIPIF.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is basically a copy/paste of the mysqli and pgsql approaches. Only a few editorial decisions were made:
skipifconnectfailure.incwithin the pre-existingskipif.inc. The check inskipif.incis for one specific condition (the type of the database) and is not universal; keeping them separate results in us having one logical "skip" check per include file. This comes at the expense of a few extra lines, but I think the end result is cleaner, and would be especially nice ifskipif.incwas renamed to something likeskipifwrongdbtype.inc.$dbase(fromdetails.inc) tonullif it is unset. As a result, twoempty()checks inext/oci8/tests/password.phptandext/oci8/tests/password_2.phpthave to be changed tois_null().