-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add more test coverage for ext/ODBC #11938
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
Conversation
kocsismate
commented
Aug 11, 2023
- adds tests for many functions which used to be untested
- enable tests on Linux with unixODBC
- fix (mostly xfail or skip) failing tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Main comments are about reducing the number of insertions queries and being confused at the inconsistent handling of the row
parameter.
I also wonder if it makes sense to (in a follow-up) PR to enhance the warnings about invalid row numbers (if too larg) to use the appropriate function to check if the row number queried is actually in range? This could allow us to convert that case to a ValueError in the future.
Additionally, I managed to get rid of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is fine if CI is happy
var_dump(odbc_fetch_into($res, $arr, 0)); | ||
var_dump($arr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so that's what the 0
does
|
||
Warning: odbc_field_len(): Field index larger than number of fields in %s on line %d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One should know the database schema one is dealing with tho?
Also odbc_num_fields()
exists, but that's an aside.
ef03a2d
to
58e86d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit but LGTM now
ext/odbc/tests/bug69354.phpt
Outdated
--EXPECT-- | ||
100 | ||
a | ||
a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: EOL before EOF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
Some test failures are fixed, parallelization is enabled, section order is fixed.