Skip to content

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

Merged
merged 4 commits into from
Aug 23, 2023
Merged

Conversation

kocsismate
Copy link
Member

  • adds tests for many functions which used to be untested
  • enable tests on Linux with unixODBC
  • fix (mostly xfail or skip) failing tests

Copy link
Member

@Girgias Girgias left a 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.

@iluuu1994 iluuu1994 removed their request for review August 12, 2023 14:25
@kocsismate
Copy link
Member Author

Additionally, I managed to get rid of the CONFLICTS file (adding the --conflicts-- section was still necessary for 2 files).

Copy link
Member

@Girgias Girgias left a 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

Comment on lines +28 to +24
var_dump(odbc_fetch_into($res, $arr, 0));
var_dump($arr);
Copy link
Member

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
Copy link
Member

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.

@kocsismate kocsismate force-pushed the odbc-tests branch 3 times, most recently from ef03a2d to 58e86d1 Compare August 23, 2023 08:53
Copy link
Member

@Girgias Girgias left a 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

--EXPECT--
100
a
a
Copy link
Member

Choose a reason for hiding this comment

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

Nit: EOL before EOF

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

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