Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add missing dependency of database tests on backend#36073

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
hramrach wants to merge4 commits intogo-gitea:main
base:main
Choose a base branch
Loading
fromhramrach:testdep

Conversation

@hramrach
Copy link
Contributor

Missing make file dependency:

git clonehttps://github.com/go-gitea/gitea.git
make -C gitea test-sqlite

go test -c code.gitea.io/gitea/tests/integration -o integrations.sqlite.test -tags ' sqlite sqlite_unlock_notify'
sed -e 's|{{REPO_TEST_DIR}}||g'
-e 's|{{TEST_LOGGER}}|test,file|g'
-e 's|{{TEST_TYPE}}|integration|g'
tests/sqlite.ini.tmpl > tests/sqlite.ini
GITEA_ROOT="/home/hramrach/gitea" GITEA_CONF=tests/sqlite.ini ./integrations.sqlite.test
Could not find gitea binary at /home/hramrach/gitea/gitea

make: *** [Makefile:506: test-sqlite] Error 1

@GiteaBotGiteaBot added the lgtm/need 2This PR needs two approvals by maintainers to be considered for merging. labelDec 2, 2025
@hramrachhramrach changed the titleTestdepAdd missing dependency of database tests on backendDec 2, 2025
@GiteaBotGiteaBot added lgtm/need 1This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2This PR needs two approvals by maintainers to be considered for merging. labelsDec 2, 2025
@silverwind
Copy link
Member

CI failures are unrelated and will be fixed with#36074.

@silverwind
Copy link
Member

CI failures are unrelated and will be fixed with#36074.

Actually no, not entirely unrelated.go: -race requires cgo; enable cgo by setting CGO_ENABLED=1 is related.

delvh
delvh previously approved these changesDec 3, 2025
@GiteaBotGiteaBot added lgtm/doneThis PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1This PR needs approval from one additional maintainer to be merged. labelsDec 3, 2025
@wxiaoguang
Copy link
Contributor

Well, I don't think this "fix" is right and can be approved as the current implementation.

@silverwind@delvh , see the CI failures. There are more details.

delvh reacted with thumbs up emoji

@wxiaoguangwxiaoguang marked this pull request as draftDecember 3, 2025 04:07
@GiteaBotGiteaBot added lgtm/need 1This PR needs approval from one additional maintainer to be merged. and removed lgtm/doneThis PR has enough approvals to get merged. There are no important open reservations anymore. labelsDec 3, 2025
@hramrach
Copy link
ContributorAuthor

hramrach commentedDec 3, 2025•
edited
Loading

Interesting.

Clearly the dependency is real because the workflow in question first builds backend, and then runs the test.

However, the test is run with environment variables set that fail the backend build.

My suggestion would be to drop the CGO_ENABLED=0 which would then make the backend buildable as part of the test but I am not expert on go and don't know what this was supposed to accomplish.

@hramrach
Copy link
ContributorAuthor

I see. cgo, at least the version you use is broken on Arm due to bogus inline assembly.

@silverwind
Copy link
Member

silverwind commentedDec 3, 2025•
edited
Loading

I think we may just need to addCGO_ENABLED = 1 after below line because therace detector depends on it andRACE_ENABLED should only ever be set during test runs:

GOTESTFLAGS += -race

@hramrachhramrachforce-pushed thetestdep branch 5 times, most recently from42ab49e to7835f8bCompareDecember 3, 2025 23:07
CGO is required for tests that use -race as well as some other buildoptions.On arm64 the build fails when CGO is enabled because of some assemblyerror.
fixes this error: make test-sqlitego test  -c code.gitea.io/gitea/tests/integration -o integrations.sqlite.test -tags ' sqlite sqlite_unlock_notify'sed -e 's|{{REPO_TEST_DIR}}||g' \-e 's|{{TEST_LOGGER}}|test,file|g' \-e 's|{{TEST_TYPE}}|integration|g' \tests/sqlite.ini.tmpl > tests/sqlite.iniGITEA_ROOT="/home/hramrach/gitea" GITEA_CONF=tests/sqlite.ini ./integrations.sqlite.testCould not find gitea binary at /home/hramrach/gitea/giteamake: *** [Makefile:506: test-sqlite] Error 1
No need to build it explicitly, db tests now depend on it.
@hramrachhramrach marked this pull request as ready for reviewDecember 4, 2025 10:01
@silverwind
Copy link
Member

Timeout after 50m seems odd as the db tests usually take only 30m, I've restarted the failing job, maybe it was a flake.

@silverwind
Copy link
Member

silverwind commentedDec 5, 2025•
edited
Loading

If the postgres test times out again, try raising this temporarily:

.github/workflows/pull-db-tests.yml:56:        timeout-minutes: 50

@wxiaoguang
Copy link
Contributor

wxiaoguang commentedDec 5, 2025•
edited
Loading

If the postgres test times out again, try raising this temporarily:

.github/workflows/pull-db-tests.yml:56:        timeout-minutes: 50

No, it shouldn't and don't do that. There must be a bug (eg: deadlock) if it timeouts.

@silverwind
Copy link
Member

silverwind commentedDec 5, 2025•
edited
Loading

It's still valuable if we see the output of a full test run that is not cancelled, otherwise you have no full picture of the errors to work with. Will of course be reverted before merge.

@wxiaoguang
Copy link
Contributor

It's still valuable if we see the output of a full test run that is not cancelled, otherwise you have no full picture of the errors to work with. Will of course be reverted before merge.

For testing-only purpose, you can try. But I guess it won't really help, because usually a test finishes in 20-30 minutes. So you already have the doubled time.

@silverwind
Copy link
Member

silverwind commentedDec 5, 2025•
edited
Loading

Yeah i don't know why it's taking so long now, I assume it's related toRACE_ENABLED which makesgo test a lot more slow. But to my knowledge, this flag should have already been active for CI tests.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@silverwindsilverwindsilverwind approved these changes

@delvhdelvhdelvh left review comments

Assignees

No one assigned

Labels

lgtm/need 1This PR needs approval from one additional maintainer to be merged.modifies/internal

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hramrach@silverwind@wxiaoguang@delvh@GiteaBot
[8]ページ先頭
©2009-2025 Movatter.jp