test(general): assert web redirects from Appwrite instead of following them - #13712
Conversation
…nsole Call Appwrite directly with the API host and without following redirects, so the OAuth2 callback, root and blocked-user web requests assert the 301 and Location Appwrite returns rather than the console served behind the proxy.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
 |
|
|
||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertEquals(301, $response['headers']['status-code']); | ||
| $this->assertEquals('http://localhost/?project=console', $response['headers']['location']); |
There was a problem hiding this comment.
This exact http://localhost assertion mirrors the test deployment’s configured console URL instead of isolating the observable redirect behavior. The console origin and scheme can come from _APP_CONSOLE_URL or related environment settings, so a valid configuration change can fail this test even when Appwrite still redirects correctly. This violates the repository directive not to mirror configuration in assertions and must be fixed before merging. Assert the redirect semantics and preserved path/query without hardcoding the configured origin. The same coupling appears in the other hook and OAuth callback assertions.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/e2e/General/HooksTest.php
Line: 63
Comment:
**Configured Origin Is Hardcoded**
This exact `http://localhost` assertion mirrors the test deployment’s configured console URL instead of isolating the observable redirect behavior. The console origin and scheme can come from `_APP_CONSOLE_URL` or related environment settings, so a valid configuration change can fail this test even when Appwrite still redirects correctly. This violates the repository directive not to mirror configuration in assertions and must be fixed before merging. Assert the redirect semantics and preserved path/query without hardcoding the configured origin. The same coupling appears in the other hook and OAuth callback assertions.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What does this PR do?
Since #13689,
/,/auth/*and the other legacy console paths return a 301 to the console URL. The General e2e tests for the OAuth2 callbacks and the web hooks (project=console, unknown project, blocked user) calledhttp://appwrite.testand followed the redirect. In appwrite CI that landed on the console container behind Traefik, so the 200 never came from Appwrite. In cloud CI it ran into the redirect loop guard instead (see appwrite-labs/cloud#5856).The tests now call Appwrite directly on
http://localhostwithHost: appwrite.test, without following redirects, and assert the 301 status andLocationthat Appwrite returns.testConsoleServedis intentionally unchanged.Test Plan
testConsoleServedis taggedconsoleProxy: it checks that a proxy serves the console at/, so stacks without one (appwrite-labs/cloud CI) can exclude it by group instead of by method name.