feat: e2e tests using docker compose #1165
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
d351d4b to
b5ed561
Compare
7765858 to
f3f02db
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1165 +/- ##
==========================================
- Coverage 80.65% 80.42% -0.23%
==========================================
Files 65 65
Lines 4575 4619 +44
Branches 774 775 +1
==========================================
+ Hits 3690 3715 +25
- Misses 870 889 +19
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dcoric
left a comment
There was a problem hiding this comment.
Excellent work! This is a comprehensive and well-architected E2E testing implementation.
I really appreciate the complete testing infrastructure with Docker and CI/CD - this addresses a common pain point I've experienced where tests would fail due to incorrect Node.js versions on different machines. Having everything containerized with consistent environments is a huge improvement for reliability.
The only area I'd suggest improving is the async configuration loading in src/ui/services/*.js, which currently has race conditions. This could potentially create behavioral issues if API calls are made before the runtime configuration is fully loaded, leading to requests being sent to incorrect endpoints.
Overall, this is solid work that will significantly improve our testing capabilities! 🎉
03c1460 to
4e82a52
Compare
|
@dcoric Can I get your 👀 on the API base URL handling? This should fix a number of unrelated issues regarding production deployments and it's a bit more robust now. Had to add a bit more logic for handling CORS properly as well. Thanks! cc @jescalada @kriswest (sorry for the delays on this one) Informing the rest of the maintainer team. There are "production deployment" related changes in this PR around CORS and base URL handling on the frontend required to make this testing setup work so hopefully we can merge that smoothly and add it to 2.x. |
coopernetes
left a comment
There was a problem hiding this comment.
Some changes of note that I would appreciate a set of 👀 on. Thanks!
dcoric
left a comment
There was a problem hiding this comment.
LGTM!
The changes to use Vite's built-in terser configuration are a solid improvement and clean up the build process effectively.
24b91e4 to
b531bd4
Compare
|
@coopernetes I’ve checked that you’ve already updated the PR with the latest branch. Once I get the approval, I can go ahead and merge it and it should update automatically, right? Since all the checks have passed, I think we only need the admin’s approval now. |
Hi @sidshas03 we are awaiting a maintainer to approve this PR. Your prior changes via #1199 were cherry-picked onto this branch so there is no action required on your side. Since #1199 incorporated commits from this PR, it was a duplicate. Hence why I re-integrated your changes here. Once merged, both sets of combined changes will be merged into |
This comment has been minimized.
This comment has been minimized.
1e1caa4 to
3af93cd
Compare
- Remove verbose logging from Dockerfile - pin dependent actions in new e2e workflow to their respective commits - refactor the tests to work slightly more robustly (handle creds, etc)
- Removed the obsolete 'version: 3.7' field from docker-compose.yml - This fixes the warning about the version field being obsolete in newer Docker Compose versions - The e2e tests now run without warnings
- Fix OIDC configuration syntax errors - Update Cypress baseUrl to use correct port (8080) - Fix CI workflow to use correct port and remove & from start command - Ensure frontend is built before running tests - CSRF protection already properly disabled in test environment Cypress tests now pass: - autoApproved.cy.js: 1/1 passing - login.cy.js: 8/8 passing - repo.cy.js: failing due to rate limiting (separate issue) This resolves the main issues mentioned in the failing job: - CSRF Token Missing Errors: Fixed by proper test environment config - Shell Script Syntax Error: Fixed by removing & from start command - Unknown Authentication Strategy: Fixed OIDC syntax errors - Route Not Hit: Fixed by building frontend and using correct port
- Fix CSRF protection by setting NODE_ENV=test in CI - Fix OIDC strategy by checking enabled flag before configuration - Fix port configuration by using correct server port (8080) - Add start:ci script to run server only (not dev client) - Set CYPRESS_BASE_URL environment variable for consistency This should resolve: - CSRF token missing errors in CI - Unknown authentication strategy errors - Port mismatch issues (3000 vs 8080) - Shell script syntax errors with & character
3af93cd to
cf1c802
Compare
|
@kriswest @jescalada this is ready to merge now. One last 👀 would be appreciated! |
There was a problem hiding this comment.
Running npm run cypress:run seems to fail on my end - and the UI isn't displaying as expected on dev either:
Details
I'll look into this in more detail as it might be a problem with my device or environment.
Seems things are executing fine on my Linux environment... Likely an issue related to #1318, will link this there.
cypress/e2e/repo.cy.js
Outdated
| it('Prevents regular users from adding repos', () => { | ||
| cy.get('[data-testid="repo-list-view"]') | ||
| // Set up intercepts before visiting the page | ||
| cy.intercept('GET', '**/api/auth/me').as('authCheck'); |
There was a problem hiding this comment.
The /api/auth/me endpoint was removed since it was a duplicate of /api/auth/profile. The other usages should be removed as well!
There was a problem hiding this comment.
I'm wondering how these tests are passing if the /api/auth/me endpoint is gone (and hasn't been added accidentally during merge conflict resolution)... 🤔
There was a problem hiding this comment.
co-pilot had some suggestions on this:
Your test is likely not hitting your Express API for
/api/auth/me. Instead, a fallback (SPA dev server or reverse proxy) is returning a 200 (or 304) with an HTML page (e.g.,index.html). Since your Cypress assertion only checks the status code and accepts 200/304, the test passes—even though the/api/auth/meAPI route does not exist.Here’s how that can happen and how to confirm it.
Why a non-existent route can still yield 200/304
Several common setups can cause this:
SPA fallback to
index.html
- If your UI dev server (e.g., Vite, CRA, Next’s static export) or NGINX is fronting the UI and not proxying
/api/auth/*correctly, any unknown path like/api/auth/mecan be served by the SPA’s catch‑all, returning 200 OK (or 304 Not Modified if cached).- Result: The response is HTML, not JSON, but your test only checks
200/304.Reverse proxy misrouting
- If your proxy rules don’t route
/api/auth/*to the backend, the request stays at the UI host and the SPA serves it (same effect as Simple github-proxy Installation Instructions #1).Caching leading to 304
- If a previous run returned 200 with
index.htmlfor/api/auth/meand the browser has a cached copy (with ETag/Last-Modified), subsequent runs can get 304 Not Modified even though it’s not a real API.Loose intercept pattern
cy.intercept('GET', '**/api/auth/me')will match any host/path that ends with/api/auth/me. If your app makes that request to the UI origin (not the API), you’ll still intercept it and wait on it—even if it’s not JSON from your API.
How to prove this (quick checks)
Update the test temporarily to inspect the intercepted response:
cy.intercept('GET', '**/api/auth/me').as('authCheck'); cy.wait('@authCheck').then(({ response, request }) => { // Check headers and body shape cy.log('AuthCheck status', response?.statusCode); cy.log('AuthCheck content-type', response?.headers?.['content-type']); cy.log('AuthCheck body snippet', typeof response?.body === 'string' ? response.body.slice(0, 200) : JSON.stringify(response?.body)?.slice(0, 200) ); // Make this strict to see the real behavior: expect(response?.headers?.['content-type']).to.include('application/json'); // likely to FAIL });If you see
text/htmland some HTML snippet in the log/body, you’re hitting the SPA index fallback—not your API.
Why the test still passes
Your test’s only assertion on the auth-check is:
expect([200, 304]).to.include(interception.response.statusCode);This allows:
- 200 with HTML (index fallback), or
- 304 from cache,
So the test continues and then asserts UI behavior (absence of “Add repo” button), which can still hold true for a regular user regardless of whether the auth endpoint actually returned a valid JSON.
How to fix this robustly
Pick any (or a combination) of these options:
1) Make the backend expose
/me(alias to/profile)If the frontend expects
/api/auth/me, add a small alias so you do have a route:router.get('/me', async (req: Request, res: Response) => { if (!req.user) return res.status(401).send({ message: 'Not logged in' }); const userVal = await db.findUser((req.user as User).username); if (!userVal) return res.status(404).send('User not found'); res.json(toPublicUser(userVal)); });This keeps tests and frontend happy and removes ambiguity.
2) Change the frontend/tests to use
/api/auth/profileIf
/mewas never intended, update:
- Frontend calls to
/api/auth/profile.- Cypress intercepts:
cy.intercept('GET', '**/api/auth/profile').as('authCheck'); cy.wait('@authCheck').its('response.statusCode').should('be.oneOf', [200, 401, 404]); cy.wait('@authCheck') .its('response.headers.content-type') .should('include', 'application/json');(Notice we tightened assertions to ensure we’re dealing with JSON, not HTML.)
3) Fix proxying so
/api/*always goes to the backendEnsure your dev server or NGINX routes all
/api/**requests to the API server and does not fall back to the SPA on those paths. For example:
- Vite dev server
proxyconfig:server: { proxy: { '/api': 'http://localhost:8080', // your API } }- NGINX:
location /api/ { proxy_pass http://backend:8080; } location / { try_files $uri /index.html; }4) Make Cypress assertions stricter
So you catch these issues early:
cy.intercept('GET', '**/api/auth/me').as('authCheck'); cy.wait('@authCheck').then(({ response }) => { expect(response?.headers?.['content-type']).to.include('application/json'); // Optionally verify the shape of the JSON: expect(response?.body).to.have.property('username'); // Or status must be one of the real API statuses: expect([200, 401, 404]).to.include(response?.statusCode); });
TL;DR
- The test passes because
/api/auth/meis likely served by the SPA fallback (returns 200/304 HTML) rather than your API.- Your assertion accepts 200/304 no matter the content, so it doesn’t fail.
- Fix by adding
/me, or using/profile, and/or tightening intercept assertions and proxy config so/api/**always hits the backend.
kriswest
left a comment
There was a problem hiding this comment.
confused by an indefinite test...
Closes #1302
This PR adds a new end-to-end test setup using docker compose. It includes a few basic tests such as git clone (fetch) and a single push test with an expected failure. By moving end-to-end testing to a container-based setup, we can start to remove the state coupling that exists between various test suites at the moment. For example, attempting to run a single test can result in unexpected failures because it relies on setups from another test that starts a real GitProxy server, inserts some data into the database, etc. By refactoring those tests into these new e2e style tests, we can remove those dependencies and start to break apart the existing tests to focus more on the system-under-test.
In addition to this setup, a few other changes were made to support it as well as to cleanup some miscellaneous issues. If preferred, these can be moved out into their own PRs..
Remove some database initialization that was triggered for local files even though Mongo is in use.fixed via fix: defer reading of database configuration until needed to fix race #1316Related issues:
See below compose logs:
Details