Skip to content
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

unit tests: fix db_test PurgeInfoLogs by creating unique db path (#273) #324

Closed
wants to merge 1 commit into from

Conversation

Yuval-Ariel
Copy link
Contributor

create a unique db path for each new db opened in a suite. the PerThreadDBPath creates a db path based on the PID and the thread so that every test in the suite (e.g. db_test) has the exact same db path. this leads to unwanted leftovers which caused an error in PurgeInfoLogs.

create a unique db path for each new db opened in a suite.
the PerThreadDBPath creates a db path based on the PID and the thread so
that every test in the suite (e.g. db_test) has the exact same db path.
this leads to unwanted leftovers which caused an error in PurgeInfoLogs.
@Yuval-Ariel Yuval-Ariel requested a review from udi-speedb January 2, 2023 18:30
@Yuval-Ariel Yuval-Ariel self-assigned this Jan 2, 2023
@Yuval-Ariel Yuval-Ariel linked an issue Jan 2, 2023 that may be closed by this pull request
@Yuval-Ariel
Copy link
Contributor Author

currently checking if it hasnt broken any existing unit tests - https://github.com/speedb-io/speedb/actions/runs/3824428690

@Yuval-Ariel
Copy link
Contributor Author

both CI running the units tests and running the unit tests serially (J=1) on an azure instance finished successfully.

@mrambacher
Copy link
Contributor

Another solution was introduced in facebook/rocksdb#10000, but never merged. This solution allows tests to be tracked back to their test name, making it easier to find the data associated with a test.

@Yuval-Ariel
Copy link
Contributor Author

looks great, by name is definitely better. why was it not merged?

@mrambacher
Copy link
Contributor

looks great, by name is definitely better. why was it not merged?

Not sure why it was never merged. There was no discussion outside of the public comments. Feel free to merge it (or I can later in the week).

@Yuval-Ariel Yuval-Ariel requested review from mrambacher and removed request for udi-speedb January 3, 2023 14:27
@Yuval-Ariel
Copy link
Contributor Author

we've decided that @mrambacher will port the PR mentioned which does the same fix but with test name instead of a timestamp.
that PR should also close this issue #273

@Yuval-Ariel Yuval-Ariel closed this Jan 6, 2023
@Yuval-Ariel Yuval-Ariel deleted the 273-rebase-773-db_test-bug-purgeinfologs branch February 16, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants