-
-
Notifications
You must be signed in to change notification settings - Fork 557
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
test: add env ATUIN_TEST_LOCAL_TIMEOUT to control test timeout of SQLite #2337
Conversation
This make it possible to control the timeout of SQLite operations in test. And ATUIN_TEST_LOCAL_TIMEOUT defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior. SQLite operation timeout was first introduced from atuinsh#1590, including connection and store timeout. The env ATUIN_TEST_SQLITE_STORE_TIMEOUT which added by atuinsh#1703 only specify the store timeout. This commit doesn't deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT, but control it by setting its default to the new env ATUIN_TEST_LOCAL_TIMEOUT.
This commit increases the SQLite operation timeout to 2.0 to make the test pass. Details see upstream: atuinsh/atuin#2337.
This commit increases the SQLite operation timeout to 2s to make the test pass. Details see upstream: atuinsh/atuin#2337.
That would be good!
Not really. The builder is complex because the setting could come from multiple sources - env, config file, others in the future. I'd like to overhaul it a bit at some point though Otherwise, how does SQLite tend to perform on RISC? Totally cool with this change, but I'm just wondering if reads are always going to take >100ms even for simple cases |
OK, I'll push another commit for that.
Short answer: not always. RISC-V is just an ISA, performance is more related to silicon technology. AFAIK, RISC-V is at its early stage, most companies focus on embedded system, providing SBCs for testing. These devices are not powerful. And Arch Linux RISC-V is just a personal port project, although it's sponsored, I think its builder machines have limited performance, I think they are using QEMU for some builds, it may cause some performance problem as well. If you are interested in current performance status of RISC-V, you can find some articles on Phoronix, e.g., Benchmarking The First RISC-V Cloud Server: Scaleway EM-RV1 Performance - Phoronix. |
…TIMEOUT This deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT for simplicity as the new env ATUIN_TEST_LOCAL_TIMEOUT can control both connection and store timeout of SQLite in test. Details see 4d88611. Revert: atuinsh#1703.
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.
This commit increases the SQLite operation timeout to 2s to make the test pass. Details see upstream: atuinsh/atuin#2337.
This make it possible to control the timeout of SQLite operations in test. And
ATUIN_TEST_LOCAL_TIMEOUT
defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior.SQLite operation timeout was first introduced from #1590, including connection and store timeout. The env
ATUIN_TEST_SQLITE_STORE_TIMEOUT
which added by #1703 only specify the store timeout. This commit doesn't deprecateATUIN_TEST_SQLITE_STORE_TIMEOUT
, but control it by setting its default to the new envATUIN_TEST_LOCAL_TIMEOUT
.The reason is the same as #1703, but this time it fails due to connection timeout: archriscv.felixc.at/.status/log.htm?url=logs/atuin/atuin-18.3.0-1.log.
And I have two question for this commit if it's acceptable:
ATUIN_TEST_SQLITE_STORE_TIMEOUT
with the newATUIN_TEST_LOCAL_TIMEOUT
for simplicity?Settings::builder()
seems complex.Checks