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

Sqlite supports memory database for test purpose #32610

Closed
wants to merge 3 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 22, 2024

To make tests run faster, allow sqlite's PATH as a :memory: which means run sqlite memory-only. The data will disappear when Gitea or Gitea test exit.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 22, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 22, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/internal labels Nov 22, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 22, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 23, 2024
@techknowlogick techknowlogick enabled auto-merge (squash) November 23, 2024 21:29
@techknowlogick techknowlogick added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Nov 23, 2024
@lunny lunny marked this pull request as draft November 23, 2024 21:57
auto-merge was automatically disabled November 23, 2024 21:57

Pull request was converted to draft

@techknowlogick techknowlogick removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 23, 2024
@wxiaoguang
Copy link
Contributor

It is not right to use :memory:

@lunny
Copy link
Member Author

lunny commented Nov 25, 2024

It is not right to use :memory:

ref from https://github.com/mattn/go-sqlite3?tab=readme-ov-file#faq

image

@wxiaoguang
Copy link
Contributor

cache=shared is a must if you use :memory:, but cache=shared is also highly discouraged: https://www.sqlite.org/sharedcache.html

I do not see real benefits to play with them.

@lunny
Copy link
Member Author

lunny commented Nov 25, 2024

cache=shared is a must if you use :memory:, but cache=shared is also highly discouraged: sqlite.org/sharedcache.html

I do not see real benefits to play with them.

I think this is the only method to use memory because memory cannot be used with WAL. https://sqlite.org/forum/info/a44bddd65750d084

@wxiaoguang
Copy link
Contributor

But the sqlite tests are fast enough, I do not think these tricks would save more minutes.

The slowest test is still mysql

@lunny
Copy link
Member Author

lunny commented Nov 25, 2024

And I think we already used :memory: in our unit test.

x, err := xorm.NewEngine("sqlite3", "file::memory:?cache=shared&_txlock=immediate")

But I think you are right. SQLite test is not the bottleneck of tests. So I will close this pull request.

@lunny lunny closed this Nov 25, 2024
@lunny lunny deleted the lunny/sqlite_memory branch November 25, 2024 07:36
@wxiaoguang
Copy link
Contributor

And I think we already used :memory: in our unit test.

That's different. Other test uses the same connection pool.

But migration tests create 2 isolated connection pools, that's why it's difficult to play tricks with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code modifies/internal size/S Denotes a PR that changes 10-29 lines, ignoring generated files. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants