Skip to content

Refactor LDAP tests#37274

Merged
silverwind merged 2 commits intogo-gitea:mainfrom
wxiaoguang:refactor-ldap-test
Apr 18, 2026
Merged

Refactor LDAP tests#37274
silverwind merged 2 commits intogo-gitea:mainfrom
wxiaoguang:refactor-ldap-test

Conversation

@wxiaoguang
Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang commented Apr 18, 2026

Not really fix #37263, just make things better, and easy to catch more clues if it would fail again.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 18, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor Author

image

@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Apr 18, 2026
@wxiaoguang wxiaoguang requested a review from Copilot April 18, 2026 07:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the LDAP integration tests to reduce reliance on an external LDAP server by splitting scenarios into subtests and using mocks where possible, while also adjusting a few related test/helpers and admin auth-source editing behavior.

Changes:

  • Consolidates LDAP integration tests under TestAuthLDAP and gates real-LDAP-server scenarios behind TEST_LDAP.
  • Refactors HTML test helper to accept an io.Reader and improves unexpected-response logging to ignore hidden flash errors.
  • Removes AuthenticationForm.ID and redirects auth-source edit POSTs using the persisted source.ID instead.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/integration/integration_test.go Avoids logging hidden flash error messages when diagnosing unexpected responses.
tests/integration/html_helper.go Generalizes NewHTMLParser to accept any io.Reader.
tests/integration/auth_ldap_test.go Restructures LDAP tests into subtests; adds mock-based email sign-in test and adds edit-or-create auth source helper.
services/forms/auth_form.go Removes unused AuthenticationForm.ID field.
routers/web/admin/auths.go Redirects after updating an auth source using source.ID instead of form-bound ID.
modules/testlogger/testlogger.go Renames internal stdout printf helper and updates call sites.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/integration/auth_ldap_test.go
@silverwind
Copy link
Copy Markdown
Member

Added Fixes labels, assuming it's related to that issue.

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Apr 18, 2026

It doesn't make any known fix for the reported issue (btw: the issue can be closed by this, until the error happens again)

Either:

  • For unknown reason, the PR happens to have the issue fixed: good
  • The issue is still not fixed: this PR will output more details to let developers know why it fails: good
    • The logic is still the same as before: load the page, parse DOM, get the link
	hrefAuthSource, exists := doc.Find("table.table td a").Attr("href")
	if !assert.True(t, exists, "No authentication source found") {
		t.Logf("response: %s", respStr)
		return
	}

@silverwind
Copy link
Copy Markdown
Member

Removed the references then. If the failure is not reliably reproducible, we can't be sure whether any fix is right.

@wxiaoguang
Copy link
Copy Markdown
Contributor Author

wxiaoguang commented Apr 18, 2026

Removed the references then. If the failure is not reliably reproducible, we can't be sure whether any fix is right.

I don't see it's worth to keep the issue open. In recent years, it only happened once.

And even if it would happen next time, the code / stacktrace have hugely changed, the issue won't provide any useful information.

The issue should be closed.

@bircni
Copy link
Copy Markdown
Member

bircni commented Apr 18, 2026

Removed the references then. If the failure is not reliably reproducible, we can't be sure whether any fix is right.

I don't see it's worth to keep the issue open. In recent years, it only happened once.

And even if it would happen next time, the code / stacktrace have hugely changed, the issue won't provide any useful information.

The issue should be closed.

will close the issue and next time we have more for tracing

@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 Apr 18, 2026
@silverwind
Copy link
Copy Markdown
Member

yes close it, next time we have more context if it should ever fail again.

@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 Apr 18, 2026
@bircni bircni mentioned this pull request Apr 18, 2026
@silverwind silverwind enabled auto-merge (squash) April 18, 2026 19:02
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 18, 2026
@silverwind silverwind merged commit af31b9d into go-gitea:main Apr 18, 2026
26 checks passed
@GiteaBot GiteaBot added this to the 1.27.0 milestone Apr 18, 2026
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 18, 2026
@wxiaoguang wxiaoguang deleted the refactor-ldap-test branch April 19, 2026 03:24
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 20, 2026
* main: (25 commits)
  Add WebKit to e2e test matrix (go-gitea#37298)
  Don't add useless labels which will bother changelog generation (go-gitea#37267)
  Fix Repository transferring page (go-gitea#37277)
  Stabilize issue-project e2e test, increase timeout factor (go-gitea#37297)
  Fix Mermaid diagrams failing when node labels contain line breaks (go-gitea#37296)
  Add project column picker to issue and pull request sidebar (go-gitea#37037)
  Fix container auth for public instance (go-gitea#37290)
  Refactor frontend `tw-justify-between` layouts to `flex-left-right` (go-gitea#37291)
  Update Nix flake (go-gitea#37284)
  Workflow Artifact Info Hover (go-gitea#37100)
  [skip ci] Updated translations via Crowdin
  release notes for 1.26.0 (go-gitea#37282)
  Enhance GetActionWorkflow to support fallback references (go-gitea#37189)
  Refactor LDAP tests (go-gitea#37274)
  Remove `SubmitEvent` polyfill (go-gitea#37276)
  Upgrade go-git to v5.18.0 (go-gitea#37268)
  Avoid top-level await (go-gitea#37272)
  Frontend iframe renderer framework: 3D models, OpenAPI (go-gitea#37233)
  pull: Fix CODEOWNERS absolute path matching. (go-gitea#37244)
  Swift registry metadata: preserve more JSON fields and accept empty metadata (go-gitea#37254)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky LDAP Test

5 participants