Devops-6: Endpoints for operations and testing#64
Merged
agustinbarbalase merged 20 commits intodevfrom Apr 10, 2026
Merged
Conversation
fix: setup proper rollback and more defined error handleing for models service
fix: adding verbose logging to models to test in prod
nth time is the charm
fix: another attempt to fix models service
Contributor
Author
|
I need merged the PR #63. When it's merge, update this to ready for review |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the project’s acceptance-testing approach by removing the prior Go/Godog acceptance-test suites and introducing Python/Behave acceptance tests executed via Docker Compose, with CI automation to run both unit and acceptance tests on PRs and main.
Changes:
- Replaced Go/Godog acceptance tests with Python/Behave feature files + step implementations under
tests/. - Added Docker/Compose + Makefile targets to run unit tests, acceptance tests, or both, and introduced a GitHub Actions workflow to run them in CI.
- Adjusted assets service behavior for
testingenvironment and updated a models repository save call.
Reviewed changes
Copilot reviewed 35 out of 39 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/requirements.txt |
Adds Python dependencies for Behave-based acceptance tests. |
tests/Dockerfile |
Builds a dedicated container image to run Behave tests. |
tests/features/world.feature |
Updates world acceptance scenarios/messages. |
tests/features/verification.feature |
Updates verification scenario expected messages. |
tests/features/signup.feature |
Updates signup scenarios/expected messages. |
tests/features/login.feature |
Updates login scenarios/expected messages. |
tests/features/model.feature |
Updates models scenarios/expected error messages. |
tests/features/character.feature |
Updates character scenarios/expected messages and constraints. |
tests/features/refresh.feature |
Adds acceptance coverage for refresh verification code flow. |
tests/features/steps/world_steps.py |
Implements world Behave step definitions. |
tests/features/steps/model_steps.py |
Implements models Behave step definitions (multipart upload + listing). |
tests/features/steps/character_steps.py |
Implements character Behave step definitions (update name/bio + visibility). |
tests/features/steps/auth_steps.py |
Implements auth/verification/session/refresh Behave step definitions and helpers. |
run_tests.sh |
Simplifies the script to run Go unit tests only (acceptance handled separately). |
Makefile |
Adds test-unit and test-acceptance targets; updates test to run both. |
docker-compose.test.yml |
Adds python-tests service/profile and switches to DATABASE_URL. |
.github/workflows/tests.yml |
Adds CI workflow to run unit + acceptance tests using docker compose. |
Dockerfile |
Ensures the test-stage image includes run_tests.sh and migrations. |
internal/assets-service/router/router.go |
Uses on-disk bucket repository for testing environment. |
internal/assets-service/repositories/models/models.go |
Changes model persistence to pass a pointer to GORM Save. |
internal/middleware/jwt_session_test.go |
Updates fixed-token middleware unit test expectation. |
internal/world-service/acceptance-tests/world_test.go |
Removes old Go/Godog acceptance test harness. |
internal/world-service/acceptance-tests/world_steps.go |
Removes old Go/Godog world steps. |
internal/world-service/acceptance-tests/go.mod |
Removes old Go module for acceptance tests. |
internal/world-service/acceptance-tests/go.sum |
Removes old Go dependencies for acceptance tests. |
internal/players-service/acceptance-tests/players_test.go |
Removes old Go/Godog acceptance test harness. |
internal/players-service/acceptance-tests/character_steps.go |
Removes old Go/Godog character steps. |
internal/players-service/acceptance-tests/go.mod |
Removes old Go module for acceptance tests. |
internal/players-service/acceptance-tests/go.sum |
Removes old Go dependencies for acceptance tests. |
internal/authentication-service/acceptance-tests/auth_test.go |
Removes old Go/Godog acceptance test harness. |
internal/authentication-service/acceptance-tests/signup_steps.go |
Removes old Go/Godog signup steps. |
internal/authentication-service/acceptance-tests/login_steps.go |
Removes old Go/Godog login steps. |
internal/authentication-service/acceptance-tests/verification_steps.go |
Removes old Go/Godog verification steps. |
internal/authentication-service/acceptance-tests/go.mod |
Removes old Go module for acceptance tests. |
internal/authentication-service/acceptance-tests/go.sum |
Removes old Go dependencies for acceptance tests. |
internal/assets-service/acceptance-tests/assets_test.go |
Removes old Go/Godog acceptance test harness. |
internal/assets-service/acceptance-tests/model_steps.go |
Removes old Go/Godog model steps. |
internal/assets-service/acceptance-tests/go.mod |
Removes old Go module for acceptance tests. |
internal/assets-service/acceptance-tests/go.sum |
Removes old Go dependencies for acceptance tests. |
Comments suppressed due to low confidence (2)
internal/assets-service/repositories/models/models.go:33
- In Go, taking the address of the range loop variable (
&model) is unsafe: the loop variable is reused each iteration, and GORM may end up reading/mutating the same struct repeatedly (and you also won't persist any DB-populated fields back intomodelsList). Iterate by index and pass&modelsList[i](or assign to a new variable per iteration) to ensure each save targets the correct element.
for _, model := range modelsList {
result := mr.db.Conn.Save(&model)
if result.Error != nil {
logger.Logger.Errorf("REPO: Failed to upload model %s: %v", model.Id, result.Error)
return nil, result.Error
}
logger.Logger.Infof("REPO: Model uploaded (rows affected: %d): %s", result.RowsAffected, model.ToString())
tests/features/signup.feature:41
- The password-validation scenarios (AC-5a/5b/5c) now assert the error message "The email address is not valid.", which contradicts the scenario intent (password too short / missing number / missing letter). This makes the acceptance criteria ambiguous and hides whether password-specific validation is working. Consider asserting password-specific messages (and updating the API to return them) or renaming/reframing these scenarios to match the actual expected behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
maxogod
approved these changes
Apr 10, 2026
Member
maxogod
left a comment
There was a problem hiding this comment.
LGTM, but 1 small detail:
- Can we move the
run_tests.shscript into the./scripts/folder?
Contributor
Author
|
Done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes:
acceptance-testtests/docker-compose.test.ymlfor test with Pythontest.yml) for run test before merge tomainand when you push tomainrun-tests.shfor run only unit testsHow to test it
Run the next commands
And check if all is right