Skip to content

ext/dba/tests: sort expected test output - #14962

Merged
Girgias merged 4 commits into
php:masterfrom
orlitzky:sort-dbm-test-output
Jul 26, 2024
Merged

Girgias merged 4 commits into
php:masterfrom
orlitzky:sort-dbm-test-output

Conversation

@orlitzky

Copy link
Copy Markdown
Contributor

Fixes #14786 by sorting the fetched rows before echoing them, and updating the expected output to reflect the sort order.

I don't have dbm, ndbm, or Oracle db < 4 to test so hopefully the CI does.

orlitzky added 2 commits July 15, 2024 12:05
Iterating through a database with firstkey() and nextkey() is
guaranteed to retrieve all rows, but apparently not in any particular
order. This is causing a test failure for at least one user, so we
steal the sort() approach from GDBM to ensure that the output is
predictable.
The actual output is now sorted for consistency, so we need to update
the expected output as well. As a nice side effect, some differences
in the expected outputs for the various engines have been eliminated.

Closes phpGH-14786
Comment thread ext/dba/tests/setup/setup_dba_tests.inc
@cmb69

cmb69 commented Jul 15, 2024

Copy link
Copy Markdown
Member

I don't have dbm, ndbm, or Oracle db < 4 to test so hopefully the CI does.

Windows won't (only QDBM and LMDB are tested on CI, currently). I haven't checked other OSs.

This test uses a routine from ext/dba that now sorts its (actual)
output, so we have to sort the expected output here as well.

@devnexen devnexen 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.

ack for pgsql

@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.

LGTM, we probably should backport this

After doing some more digging, it looks like GDBM isn't the only
engine where the iteration order with firstkey() and nextkey()
might change unexpectedly.
@orlitzky

Copy link
Copy Markdown
Contributor Author

Thanks, I added one more commit to clarify the comment.

@Girgias
Girgias merged commit 44b0baf into php:master Jul 26, 2024
@ndossche

Copy link
Copy Markdown
Member

Alpine hit the sorting issue too: #16902 (comment)
They have disabled the test for that reason.
I think this PR should be backported, WDYT?

@Girgias

Girgias commented Nov 25, 2024

Copy link
Copy Markdown
Member

Yeah probably should

@ndossche

ndossche commented Nov 26, 2024 •

Copy link
Copy Markdown
Member

OK. I'll get to it, I'll make a PR soon and if that passes CI merge it.
EDIT: see #16950

ndossche pushed a commit to ndossche/php-src that referenced this pull request Nov 26, 2024
Alpine CI regularly fails because of the sorting order of these tests.
See php#14962 (comment)
ndossche pushed a commit that referenced this pull request Nov 26, 2024
Alpine CI regularly fails because of the sorting order of these tests.
See #14962 (comment)

Closes GH-16950.
ndossche added a commit that referenced this pull request Nov 26, 2024
* PHP-8.2:
  Backport GH-14962 to stable versions
ndossche added a commit that referenced this pull request Nov 26, 2024
* PHP-8.3:
  Backport GH-14962 to stable versions
ndossche added a commit that referenced this pull request Nov 26, 2024
* PHP-8.4:
  Backport GH-14962 to stable versions
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.

ext/dba/tests/dba_gdbm.phpt: test failure due to key ordering

5 participants