Skip to content

Add more test coverage for ext/ODBC - #11938

Merged
kocsismate merged 4 commits into
php:masterfrom
kocsismate:odbc-tests
Aug 23, 2023
Merged

Add more test coverage for ext/ODBC#11938
kocsismate merged 4 commits into
php:masterfrom
kocsismate:odbc-tests

Conversation

@kocsismate

@kocsismate kocsismate commented Aug 11, 2023

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

@kocsismate
kocsismate requested review from Girgias and cmb69 August 11, 2023 11:38
@kocsismate kocsismate mentioned this pull request Aug 11, 2023
Closed
12 tasks done
Girgias
Girgias reviewed Aug 11, 2023
View reviewed changes

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ext/odbc/tests/bug44618.phpt Resolved
Comment thread ext/odbc/tests/bug60616.phpt Resolved
Comment thread ext/odbc/tests/odbc_commit_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_exec_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_fetch_array_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_field_name_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_field_num_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_field_precision_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_num_fields_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_primarykeys_001.phpt Resolved
@iluuu1994
iluuu1994 removed their request for review August 12, 2023 14:25
@kocsismate

kocsismate commented Aug 22, 2023

Copy link
Copy Markdown
Member Author

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

Girgias
Girgias reviewed Aug 22, 2023
View reviewed changes

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

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 thread ext/odbc/tests/bug47803.phpt Resolved
Comment thread ext/odbc/tests/odbc_fetch_into_001.phpt Outdated
Comment on lines 28 to 24

@Girgias Girgias Aug 22, 2023

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

Comment thread ext/odbc/tests/odbc_fetch_object_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_field_len_001.phpt Outdated

@Girgias Girgias Aug 22, 2023

Copy link
Copy Markdown
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.

Comment thread ext/odbc/tests/odbc_free_result_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_free_result_001.phpt Resolved
Comment thread ext/odbc/tests/odbc_primarykeys_001.phpt Resolved
@kocsismate
kocsismate force-pushed the odbc-tests branch 3 times, most recently from ef03a2d to 58e86d1 Compare August 23, 2023 08:53
Girgias
Girgias approved these changes Aug 23, 2023
View reviewed changes

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

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

Comment thread ext/odbc/tests/bug69354.phpt Outdated

@Girgias Girgias Aug 23, 2023

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

@kocsismate kocsismate Aug 23, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed!

@kocsismate
kocsismate merged commit 462792e into php:master Aug 23, 2023
@kocsismate
kocsismate deleted the odbc-tests branch August 23, 2023 19:20
@jrfnl jrfnl mentioned this pull request Sep 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Girgias Girgias Girgias approved these changes
@cmb69 cmb69 Awaiting requested review from cmb69
@TimWolla TimWolla Awaiting requested review from TimWolla TimWolla is a code owner

Assignees

No one assigned

Labels

Extension: odbc Extension: pdo (core) Extension: pdo_odbc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kocsismate @Girgias