-
Notifications
You must be signed in to change notification settings - Fork 146
feat: e2e tests using docker compose #1165
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
d351d4b
to
b5ed561
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)
b5ed561
to
1e60ef5
Compare
7765858
to
f3f02db
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1165 +/- ##
==========================================
+ Coverage 82.83% 82.92% +0.09%
==========================================
Files 66 66
Lines 2784 2806 +22
Branches 334 338 +4
==========================================
+ Hits 2306 2327 +21
- Misses 431 432 +1
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🎉
let baseUrl = location.origin; // Default fallback | ||
|
||
// Set the actual baseUrl from runtime config | ||
getApiBaseUrl() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can potentially create a race condition - API calls can start before this baseUrl is resolved, and that will make them run to a default fallback.
The potential idea to resolve this could be something like this:
// Make all API calls async and wait for configuration
const getApiBaseUrl = async () => {
const config = await getRuntimeConfig();
return config.apiUrl || location.origin;
};
// Use in services:
const apiUrl = await getApiBaseUrl();
const response = await fetch(`${apiUrl}/api/v1/endpoint`);
This is in draft until the workflow is fully tested - currently underway in a private forkcompletedThis 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..
See below compose logs: