Skip to content

ext/dba/tests/dba_tcadb.phpt: fix expected output #11648

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 1 commit into from
Aug 2, 2023

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Jul 9, 2023

Several of our DBA tests (based on setup/setup_dba_tests.inc) have a "no lock" version whose expected output is,

SAME OUTPUT AS PREVIOUS RUN (modulo read during write due to no lock)

This particular tokyocabinet test, however, is missing the "modulo" bit, causing the lock-free test to fail. Comparing with, for example, ext/dba/tests/dba_{flat,ini}file.phpt, it looks like the right thing to do is simply to add it.

@Girgias
Copy link
Member

Girgias commented Jul 10, 2023

This now fail on CI and on my own machine too.

The problem with DBA is that... drivers and databases can implement stuff a bit how they want AFAIK and do not support the same feature set necessarily

@orlitzky
Copy link
Contributor Author

Hm. Should I just use a wildcard on that line then?

@orlitzky orlitzky force-pushed the fix-tcadb-test-expected-output branch from f1b0d70 to 49240fd Compare July 10, 2023 14:45
@Girgias
Copy link
Member

Girgias commented Jul 10, 2023

Hm. Should I just use a wildcard on that line then?

Do you hit a different output on your machine?

@orlitzky
Copy link
Contributor Author

Do you hit a different output on your machine?

Yeah, I get the extra (modulo read during write due to no lock). Based on other similar tests, I had assumed its absence was an oversight; I'm not really using tokyocabinet so I only noticed the test failure yesterday.

I wonder what the difference is. I've got tokyocabinet-1.4.48.

@Girgias
Copy link
Member

Girgias commented Jul 11, 2023

I don't think it's the version of the library as I also have tokyocabinet-1.4.48-21.fc38.x86_64

@orlitzky
Copy link
Contributor Author

Neither fc38 nor Gentoo have any weird TokyoCabinet patches, so who knows.

In any case, reading https://www.php.net/manual/en/function.dba-open.php, it says "read during write is not allowed" and that the no-lock mode should be used only if "you are absolutely sure that you do not require database locking." If we take that as our source of truth, then to me it sounds like we shouldn't expect anything to happen reliably if the user lies to PHP and tells it not to use a lock while attempting reads in the middle of a write.

@Girgias
Copy link
Member

Girgias commented Jul 12, 2023

The test is just a smoke screen tests that is generic to all drivers that I wrote so that I could refactor ext/dba, moreover I'm not sure relying on the written docs is that wisest thing to do as the canonical source of truth is the implementation.

But having a wildcard for the test makes sense I suppose, even tho I don't understand why you get different behaviour.

Several of our DBA tests (based on setup/setup_dba_tests.inc) have a
"no lock" version whose expected output is,

  SAME OUTPUT AS PREVIOUS RUN (modulo read during write due to no lock)

This tokyocabinet test, however, is missing the "modulo" bit, because
it is not output when tokyocabinet is built with pthreads support, as
is the case on at least Fedora and the GitHub CI.

To additionally support systems where tokyocabinet is built WITHOUT
pthreads support, this commit adds a wildcard at the end of the
expected output to catch the " (modulo..." string.
@orlitzky orlitzky force-pushed the fix-tcadb-test-expected-output branch from 49240fd to 1707937 Compare July 12, 2023 16:05
@orlitzky
Copy link
Contributor Author

I don't understand why you get different behaviour.

Found it! It changes when you build tokyocabinet with(out) --enable-pthread.

@Girgias
Copy link
Member

Girgias commented Jul 14, 2023

I don't understand why you get different behaviour.

Found it! It changes when you build tokyocabinet with(out) --enable-pthread.

Maybe it makes more sense to SKIPIF if the extension is enabled?

@orlitzky
Copy link
Contributor Author

Found it! It changes when you build tokyocabinet with(out) --enable-pthread.

Maybe it makes more sense to SKIPIF if the extension is enabled?

I suppose it would be even better to have two tests, one for threaded TokyoCabinet and one for threadless. But how would we implement the SKIPIF? The only way that I know of at the moment to detect a threadless TokyoCabinet is to run your test :)

@Girgias
Copy link
Member

Girgias commented Jul 17, 2023

Yeah... I look into a bit of Tokyo Cabinet and learned a couple of things, it got superseded twice by other libraries from the same author. And that it doesn't seem to expose via its public API if it was compiled with pthreads. And I can't seem to find how one would get access to the information stored in TCUSEPTHREAD as all the header and functions that use that are private/static...

So yeah that is annoying.

@Girgias Girgias merged commit 4140394 into php:master Aug 2, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
…1648)

Several of our DBA tests (based on setup/setup_dba_tests.inc) have a
"no lock" version whose expected output is:

  SAME OUTPUT AS PREVIOUS RUN (modulo read during write due to no lock)

This tokyocabinet test, however, is missing the "modulo" bit, because
it is not output when tokyocabinet is built with pthreads support, as
is the case on at least Fedora and the GitHub CI.

To additionally support systems where tokyocabinet is built WITHOUT
pthreads support, this commit adds a wildcard at the end of the
expected output to catch the " (modulo..." string.
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