Assessment: Add server-side-tests to assessment server
#555
Conversation
- Implemented SetupTestDB function to initialize a PostgreSQL container for testing, including running SQL dumps. - Created MockAuthMiddleware and MockPermissionMiddleware to facilitate testing with mocked authentication and permission checks.
|
Warning Rate limit exceeded@rappm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis change introduces comprehensive automated tests for the assessment server, including service and router tests for assessments, categories, competencies, score levels, and assessment completion. It adds test database setup utilities, SQL dumps for test data, updates dependencies, and restructures CI workflows to enable targeted Go server testing based on code changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions
participant TestDB as Docker PostgreSQL
participant GoTest as Go Test Runner
Dev->>CI: Open PR or push changes
CI->>CI: Detect changes in assessment server directories
alt Changes detected
CI->>TestDB: Start PostgreSQL container and load SQL dump
CI->>GoTest: Run Go tests (service & router tests)
GoTest->>TestDB: Connect and run tests using test data
GoTest->>CI: Output test results (JUnit XML)
CI->>CI: Upload test artifacts
else No changes
CI-->>Dev: Skip Go server tests
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…d update output format to JUnit XML
… exclude directive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
.gitignore (1)
12-12: Simplify and broaden.DS_Storeignore pattern
The pattern**/*/.DS_Storeonly matches.DS_Storefiles at least one directory deep and will miss root-level or directly nested instances. Consider replacing it with a single entry that covers all depths:- **/*/.DS_Store + .DS_Store + **/.DS_Storeservers/assessment/testutils/mockPermissionMiddleware.go (1)
5-9: Consider documenting the mock behavior and unused parameters.The mock middleware correctly implements a no-op behavior for testing, but consider the following improvements:
- Add a comment explaining that this is a no-op mock for testing
- The
authRolesparameter is unused - either use it for validation or document why it's ignored+// MockPermissionMiddleware returns a no-op middleware for testing that bypasses permission checks. +// The authRoles parameter is accepted to match the signature of the real middleware but is ignored. func MockPermissionMiddleware(authRoles ...string) gin.HandlerFunc { return func(c *gin.Context) { + // No-op: bypass permission checks in tests c.Next() } }servers/assessment/go.mod (1)
13-54:❓ Verification inconclusive
Verify the necessity of all new indirect dependencies.
Many new indirect dependencies were added (Docker, OpenTelemetry, system utilities). While these likely support the testcontainers functionality, ensure they're all necessary and don't introduce security risks.
🏁 Script executed:
#!/bin/bash # Description: Check for any known vulnerabilities in the new dependencies # Expected: No high-severity vulnerabilities should be found # Use go mod to check for vulnerabilities in dependencies cd servers/assessment go list -json -m all | jq -r '.Path + "@" + .Version' | head -20 # Check if go vulnerability database can scan these if command -v govulncheck &> /dev/null; then echo "Running vulnerability check..." govulncheck ./... else echo "govulncheck not available, consider installing: go install golang.org/x/vuln/cmd/govulncheck@latest" fiLength of output: 1132
Verify necessity and security of all new indirect dependencies
I wasn’t able to run a full vulnerability scan because
govulncheckisn’t installed in the CI image. Please:
Install and run the Go vulnerability checker:
go install golang.org/x/vuln/cmd/govulncheck@latest cd servers/assessment govulncheck ./...to ensure none of the newly introduced transitive dependencies has known high-severity issues.
Prune unused modules with
go mod tidyand review the resulting diff to confirm you haven’t retained any unnecessary requirements.Trace why each dep is pulled in via
go mod graph(e.g.go mod graph | grep <module>) and verify that every indirect dependency is required by your Testcontainers or other intended functionality.This will help minimize bloat and surface any security risks before merging.
servers/assessment/testutils/db_setup.go (1)
30-34: Consider making resource allocation configurable.The hardcoded resource limits (2GB memory, 2 CPUs) might be excessive for some test environments or insufficient for others. Consider making these configurable through environment variables or function parameters.
+// DefaultContainerMemory and DefaultContainerCPUs can be overridden via environment variables +const ( + DefaultContainerMemory = 2 * 1024 * 1024 * 1024 // 2GB + DefaultContainerCPUs = 2 * 1000000000 // 2 CPUs +) + func SetupTestDB(ctx context.Context, sqlDumpPath string) (*TestDB, func(), error) { + memory := DefaultContainerMemory + cpus := DefaultContainerCPUs + if memStr := os.Getenv("TEST_CONTAINER_MEMORY"); memStr != "" { + if mem, err := strconv.ParseInt(memStr, 10, 64); err == nil { + memory = mem + } + } + if cpuStr := os.Getenv("TEST_CONTAINER_CPUS"); cpuStr != "" { + if cpu, err := strconv.ParseInt(cpuStr, 10, 64); err == nil { + cpus = cpu + } + } + // Set up PostgreSQL container req := testcontainers.ContainerRequest{ Image: "postgres:15", ExposedPorts: []string{"5432/tcp"}, Env: map[string]string{ "POSTGRES_USER": "testuser", "POSTGRES_PASSWORD": "testpass", "POSTGRES_DB": "prompt", }, HostConfigModifier: func(hc *container.HostConfig) { - // allocate 2GB memory and 2 CPUs for the test container - hc.Resources.Memory = 2 * 1024 * 1024 * 1024 - hc.Resources.NanoCPUs = 2 * 1000000000 + hc.Resources.Memory = memory + hc.Resources.NanoCPUs = cpus },servers/assessment/assessments/router_test.go (3)
29-51: Good test setup but consider extracting middleware configuration.The test setup is well-structured, but the middleware configuration could be extracted for better reusability across test suites.
func (suite *AssessmentRouterTestSuite) SetupSuite() { suite.suiteCtx = context.Background() testDB, cleanup, err := testutils.SetupTestDB(suite.suiteCtx, "../database_dumps/assessments.sql") if err != nil { suite.T().Fatalf("Failed to set up test DB: %v", err) } suite.cleanup = cleanup // initialize service singleton suite.service = AssessmentService{ queries: *testDB.Queries, conn: testDB.Conn, } AssessmentServiceSingleton = &suite.service suite.router = gin.Default() api := suite.router.Group("/api/course_phase/:coursePhaseID") - testMiddleware := func(allowedRoles ...string) gin.HandlerFunc { - return testutils.MockAuthMiddlewareWithEmail(allowedRoles, "[email protected]", "[email protected]", "id") - } + testMiddleware := testutils.CreateTestMiddleware("[email protected]", "[email protected]", "id") // attach assessment routes setupAssessmentRouter(api, testMiddleware) }Consider adding a helper function in testutils:
func CreateTestMiddleware(email, matriculationNumber, universityLogin string) func(allowedRoles ...string) gin.HandlerFunc { return func(allowedRoles ...string) gin.HandlerFunc { return MockAuthMiddlewareWithEmail(allowedRoles, email, matriculationNumber, universityLogin) } }
59-71: Add more detailed assertions for the response content.While the test checks for a successful response and non-empty array, it could benefit from more specific assertions about the assessment data structure.
func (suite *AssessmentRouterTestSuite) TestListByCoursePhase() { phaseID := uuid.MustParse("24461b6b-3c3a-4bc6-ba42-69eeb1514da9") req, _ := http.NewRequest("GET", "/api/course_phase/"+phaseID.String()+"/student-assessment", nil) resp := httptest.NewRecorder() suite.router.ServeHTTP(resp, req) assert.Equal(suite.T(), http.StatusOK, resp.Code) var items []assessmentDTO.Assessment err := json.Unmarshal(resp.Body.Bytes(), &items) assert.NoError(suite.T(), err) assert.Greater(suite.T(), len(items), 0) + + // Verify the structure of returned assessments + for _, item := range items { + assert.NotEmpty(suite.T(), item.ID, "Assessment ID should not be empty") + assert.NotEmpty(suite.T(), item.CoursePhaseID, "Course phase ID should not be empty") + // Add more field validations as needed + } }
97-102: Consider testing specific invalid UUID scenarios.The test uses a generic "invalid" string but could be more specific about different types of invalid UUIDs to ensure comprehensive error handling.
func (suite *AssessmentRouterTestSuite) TestInvalidUUIDs() { - req, _ := http.NewRequest("GET", "/api/course_phase/invalid/student-assessment/invalid", nil) - resp := httptest.NewRecorder() - suite.router.ServeHTTP(resp, req) - assert.Equal(suite.T(), http.StatusBadRequest, resp.Code) + testCases := []struct { + name string + phaseID string + endpoint string + }{ + {"invalid_phase_id", "invalid", "/student-assessment"}, + {"empty_phase_id", "", "/student-assessment"}, + {"invalid_participation_id", uuid.New().String(), "/student-assessment/invalid"}, + } + + for _, tc := range testCases { + suite.T().Run(tc.name, func(t *testing.T) { + req, _ := http.NewRequest("GET", "/api/course_phase/"+tc.phaseID+tc.endpoint, nil) + resp := httptest.NewRecorder() + suite.router.ServeHTTP(resp, req) + assert.Equal(t, http.StatusBadRequest, resp.Code) + }) + } }servers/assessment/assessments/scoreLevel/service_test.go (1)
42-51: Add validation for score level enum values.While the test checks for non-empty score levels, it should validate that the returned values are valid enum constants.
func (suite *ScoreLevelServiceTestSuite) TestGetAllScoreLevels() { phaseID := uuid.MustParse("24461b6b-3c3a-4bc6-ba42-69eeb1514da9") levels, err := GetAllScoreLevels(suite.suiteCtx, phaseID) assert.NoError(suite.T(), err) assert.Greater(suite.T(), len(levels), 0, "Expected at least one score level") + + validScoreLevels := map[db.ScoreLevel]bool{ + db.ScoreLevelNovice: true, + db.ScoreLevelIntermediate: true, + // Add other valid levels as needed + } + for _, lvl := range levels { assert.NotEmpty(suite.T(), lvl.CourseParticipationID, "Participation ID should not be empty") assert.NotEmpty(suite.T(), string(lvl.ScoreLevel), "Score level should not be empty") + assert.True(suite.T(), validScoreLevels[lvl.ScoreLevel], "Score level should be a valid enum value: %s", lvl.ScoreLevel) } }servers/assessment/categories/router_test.go (2)
61-65: Enhance response validation in category listing test.The test only checks that categories are returned but doesn't validate the structure or content. Consider adding more specific assertions about category fields.
var cats []categoryDTO.Category err := json.Unmarshal(resp.Body.Bytes(), &cats) assert.NoError(suite.T(), err) assert.Greater(suite.T(), len(cats), 0, "Should return a list of categories") + + // Validate category structure + for _, cat := range cats { + assert.NotEmpty(suite.T(), cat.ID, "Category ID should not be empty") + assert.NotEmpty(suite.T(), cat.Name, "Category name should not be empty") + assert.Greater(suite.T(), cat.Weight, int32(0), "Category weight should be positive") + }
91-95: Make error message validation more specific.The current error validation only checks for the presence of an "error" key. Consider validating the specific error message for better test coverage.
var errResp map[string]string err := json.Unmarshal(resp.Body.Bytes(), &errResp) assert.NoError(suite.T(), err) - assert.Contains(suite.T(), errResp, "error") + assert.Contains(suite.T(), errResp, "error") + assert.Contains(suite.T(), errResp["error"], "invalid", "Error message should indicate invalid JSON")servers/assessment/competencies/service_test.go (1)
18-18: Remove unnecessary router setup in service tests.This is a service test suite but includes router setup that isn't used. Service tests should focus on business logic without HTTP concerns.
type CompetencyServiceTestSuite struct { suite.Suite - router *gin.Engine ctx context.Context cleanup func() competencyService CompetencyService } CompetencyServiceSingleton = &suite.competencyService - suite.router = gin.Default() }Also applies to: 39-39
servers/assessment/database_dumps/competencies.sql (1)
198-198: Consider documenting the schema migration version.The schema migration version (8) indicates this is the 8th migration, but there's no documentation about what this version represents or what changes it includes.
Consider adding comments to document what migration version 8 encompasses:
+-- Migration version 8: Initial assessment system schema with competencies, categories, and scoring INSERT INTO public.schema_migrations VALUES (8, false);servers/assessment/database_dumps/assessments.sql (1)
361-361: Consider adding indexes for query performance.While there's an index on
assessment_completion, consider adding indexes on frequently queried columns in theassessmenttable for better performance with large datasets.-- Add indexes for common query patterns CREATE INDEX idx_assessment_course_participation_phase ON public.assessment USING btree (course_participation_id, course_phase_id); CREATE INDEX idx_assessment_competency ON public.assessment USING btree (competency_id); CREATE INDEX idx_assessment_author_timestamp ON public.assessment USING btree (author, assessed_at);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
servers/assessment/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/go-tests.yml(0 hunks).github/workflows/lint-test-prod.yml(1 hunks).github/workflows/test-servers.yml(1 hunks).gitignore(1 hunks)servers/assessment/assessments/assessmentCompletion/router_test.go(1 hunks)servers/assessment/assessments/assessmentCompletion/service_test.go(1 hunks)servers/assessment/assessments/router_test.go(1 hunks)servers/assessment/assessments/scoreLevel/router_test.go(1 hunks)servers/assessment/assessments/scoreLevel/service_test.go(1 hunks)servers/assessment/assessments/service_test.go(1 hunks)servers/assessment/categories/router_test.go(1 hunks)servers/assessment/categories/service_test.go(1 hunks)servers/assessment/competencies/router_test.go(1 hunks)servers/assessment/competencies/service_test.go(1 hunks)servers/assessment/database_dumps/assessmentCompletions.sql(1 hunks)servers/assessment/database_dumps/assessments.sql(1 hunks)servers/assessment/database_dumps/categories.sql(1 hunks)servers/assessment/database_dumps/competencies.sql(1 hunks)servers/assessment/go.mod(2 hunks)servers/assessment/testutils/db_setup.go(1 hunks)servers/assessment/testutils/mockAuthMiddleware.go(1 hunks)servers/assessment/testutils/mockPermissionMiddleware.go(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/go-tests.yml
🧰 Additional context used
🧬 Code Graph Analysis (3)
servers/assessment/assessments/assessmentCompletion/service_test.go (1)
servers/assessment/assessments/assessmentCompletion/service.go (6)
AssessmentCompletionService(18-21)AssessmentCompletionServiceSingleton(23-23)CountRemainingAssessmentsForStudent(37-47)UnmarkAssessmentAsCompleted(87-97)ListAssessmentCompletionsByCoursePhase(99-106)GetAssessmentCompletion(108-118)
servers/assessment/assessments/scoreLevel/service_test.go (2)
servers/assessment/assessments/scoreLevel/service.go (5)
ScoreLevelService(13-16)ScoreLevelServiceSingleton(18-18)GetAllScoreLevels(20-29)GetScoreLevelByCourseParticipationID(31-42)GetStudentScore(44-63)servers/assessment/db/sqlc/models.go (3)
ScoreLevel(15-15)ScoreLevelNovice(18-18)ScoreLevelIntermediate(19-19)
servers/assessment/competencies/service_test.go (1)
servers/assessment/competencies/service.go (8)
CompetencyService(17-20)CompetencyServiceSingleton(22-22)ListCompetencies(65-72)GetCompetency(56-63)ListCompetenciesByCategory(74-81)CreateCompetency(24-54)UpdateCompetency(83-100)DeleteCompetency(102-109)
🪛 actionlint (1.7.7)
.github/workflows/test-servers.yml
16-16: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-servers / assessment / merge
- GitHub Check: build-servers / team-allocation / merge
- GitHub Check: build-servers / core / merge
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
🔇 Additional comments (44)
servers/assessment/database_dumps/assessmentCompletions.sql (6)
40-45: LGTM: Well-defined enum type for competency scoring.The
score_levelenum provides a clear progression hierarchy for competency assessment, which aligns well with educational assessment practices.
70-75: LGTM: Assessment completion table design is appropriate.The composite primary key on
(course_participation_id, course_phase_id)ensures one completion record per participant per phase, which is the expected business logic.
179-181: LGTM: Unique constraint prevents duplicate assessments.The unique constraint on
(course_participation_id, course_phase_id, competency_id)appropriately prevents duplicate assessments for the same competency within a participant's phase.
226-228: LGTM: Proper foreign key constraints with cascading deletes.The foreign key constraints with
ON DELETE CASCADEprovide good data integrity while allowing clean removal of parent records.Also applies to: 234-236
54-63:❓ Verification inconclusive
Verify the assessment table design for completeness.
The assessment table structure looks well-designed with proper UUID usage and timestamps. However, consider a few points:
- The
authorfield usestexttype - consider if this should reference a user table instead for better data integrity- The default empty string for
authormight not be ideal - consider making it nullable or requiring a valueLet me check if there are any user/author tables in the related database dumps:
🏁 Script executed:
#!/bin/bash # Description: Check for user/author table references in related database dumps # Expected: Find if there are user tables that the author field should reference # Search for user-related tables in the assessment directory fd -e sql . servers/assessment/database_dumps/ --exec grep -l -i "user\|author\|participant"Length of output: 304
Verify the author field for a user reference
I searched the SQL dumps under
servers/assessment/database_dumpsbut did not find anyusers(or equivalent) table definition. Before finalizing theauthorcolumn as plaintext:• Confirm whether you have a centralized
users(orauthors) table elsewhere in your schema.
• If so, changeauthor text→author uuidand add aREFERENCES users(id)foreign-key.
• If not, consider renamingauthorto something likeauthor_name, decide whether it should beNOT NULLor nullable, and document its intended use.
122-129:✅ Verification successful
Verify consistency of sample assessment data.
The sample assessment data looks realistic, but ensure the referenced UUIDs exist in their respective tables for referential integrity during testing.
🏁 Script executed:
#!/bin/bash # Description: Verify that UUIDs in assessment data match competency IDs # Expected: All competency_id values should exist in the competency data # Extract competency IDs from the assessment inserts and check against competency table data echo "Competency IDs referenced in assessments:" grep "INSERT INTO public.assessment VALUES" servers/assessment/database_dumps/assessmentCompletions.sql | \ sed -E "s/.*'([^']+)', '[^']+', '[^']+', '([^']+)'.*/\2/" | sort -u echo -e "\nCompetency IDs defined in competency table:" grep "INSERT INTO public.competency VALUES" servers/assessment/database_dumps/assessmentCompletions.sql | \ sed -E "s/.*'([^']+)'.*/\1/" | sort -uLength of output: 1130
I’ll verify against the actual competency dump files across the
servers/assessment/database_dumpsdirectory:
🏁 Script executed:
#!/bin/bash set -eo pipefail # 1) Find files with competency inserts echo "Competency dump files:" rg -l "INSERT INTO public.competency" -g "*.sql" servers/assessment/database_dumps # 2) Extract competency IDs referenced by assessments echo -e "\nAssessment → Competency IDs:" grep "INSERT INTO public.assessment VALUES" servers/assessment/database_dumps/assessmentCompletions.sql \ | awk -F"'" '{print $8}' \ | sort -u # 3) Extract all competency IDs defined in the dumps echo -e "\nDefined Competency IDs:" grep "INSERT INTO public.competency" servers/assessment/database_dumps/*.sql \ | awk -F"'" '{print $2}' \ | sort -u # 4) Show any missing references echo -e "\nMissing competency IDs in the competency dumps:" comm -23 \ <(grep "INSERT INTO public.assessment VALUES" servers/assessment/database_dumps/assessmentCompletions.sql \ | awk -F"'" '{print $8}' | sort -u) \ <(grep "INSERT INTO public.competency" servers/assessment/database_dumps/*.sql \ | awk -F"'" '{print $2}' | sort -u)Length of output: 2130
Competency ID consistency confirmed
All seven unique
competency_idvalues inserted inassessmentCompletions.sqlare defined in your competency dump files—there are no missing references. Referential integrity for these sample assessment rows is intact..github/workflows/lint-test-prod.yml (1)
13-13:✅ Verification successful
Verify the new test workflow provides equivalent functionality.
The change from
go-tests.ymltotest-servers.ymlaligns with the PR objective of adding targeted server-side tests. Ensure the new workflow properly handles the assessment server tests.
🏁 Script executed:
#!/bin/bash # Description: Verify the new test-servers.yml workflow exists and check its content # Expected: The workflow should exist and include assessment server testing # Check if the new workflow file exists ls -la .github/workflows/test-servers.yml # Show the content of the new workflow to verify it handles assessment server tests cat .github/workflows/test-servers.ymlLength of output: 2340
New test-servers.yml workflow verified
The
.github/workflows/test-servers.ymlfile exists and includes adetect-changesstep filtering onservers/assessment/**, and itstestjob correctly runs Go tests (viagotestsum) against the assessment directory when changes are detected. No further action required.servers/assessment/go.mod (2)
6-6: LGTM: Minor version updates for core dependencies.The updates to Gin (v1.10.0 → v1.10.1) and pgx (v5.7.4 → v5.7.5) are patch versions that typically include bug fixes and security improvements.
Also applies to: 8-8
83-83:✅ Verification successful
Verify testcontainers version for security vulnerabilities.
The addition of testcontainers-go v0.37.0 enables integration testing with containerized databases, which aligns well with the comprehensive test suite being added.
🌐 Web query:
What is the latest version of testcontainers-go and are there any known security vulnerabilities in version 0.37.0?💡 Result:
The latest released version of testcontainers-go is v0.37.0, according to the official GitHub releases page[1].
Regarding security vulnerabilities in version 0.37.0:
- The release notes for v0.37.0 mention a security update that bumps the dependency github.com/containerd/containerd from version 1.7.12 to 1.7.15, addressing known security concerns in containerd[1]. This indicates that v0.37.0 includes an upgrade to a safer containerd version.
- Previously, versions around v0.20.2 to v0.27.0 had issues with vulnerabilities related to older containerd and runc versions, particularly CVE-2024-21626 (container breakout vulnerability), but these issues were fixed in later versions starting from v0.28.0 onward[7].
- No explicit security vulnerabilities are reported against v0.37.0 itself, and the dependency bump suggests that known container runtime security issues have been addressed.
In summary:
Aspect Details Latest version v0.37.0 Security status of v0.37.0 Includes containerd bump to 1.7.15 to fix CVEs; no known unresolved vulnerabilities reported Past vulnerabilities Older versions (≤ v0.27.0) had issues fixed in v0.28.0+ Thus, v0.37.0 is the latest version and has updated dependencies to mitigate known container runtime vulnerabilities, with no reported new security issues[1][7].
Citations:
- 1: https://github.com/testcontainers/testcontainers-go/releases
- 2: https://github.com/testcontainers/testcontainers-go
- 3: https://pkg.go.dev/github.com/testcontainers/testcontainers-go
- 4: https://golang.testcontainers.org
- 5: https://blog.stackademic.com/comprehensive-guide-to-testcontainers-for-go-practical-testing-with-containers-b11039e0bf41
- 6: https://docs.konghq.com/mesh/changelog/
- 7: [Bug]: runc, container breakout vulnerabilities (CVE-2024-21626) testcontainers/testcontainers-go#2198
- 8: https://www.docker.com/blog/creating-modules-in-testcontainers-go/
- 9: https://snyk.io/advisor/golang/github.com/testcontainers/testcontainers-go/modules/chroma
- 10: https://security.snyk.io/package/npm/testcontainers
Security verification for testcontainers-go v0.37.0 completed
Testcontainers-go v0.37.0 is the latest release and includes an upgrade of its containerd dependency to v1.7.15—addressing known CVEs (e.g. CVE-2024-21626). No unresolved security issues have been reported against v0.37.0.
Change approved.servers/assessment/testutils/mockAuthMiddleware.go (2)
8-26: LGTM! Clean mock authentication implementation.The middleware correctly sets up mock user context data for testing purposes. The implementation is straightforward and follows good practices for test utilities.
28-30: Good convenience wrapper function.The simplified
MockAuthMiddlewarefunction provides a clean API for tests that don't need specific user details, defaulting to empty strings for email and other fields.servers/assessment/testutils/db_setup.go (1)
83-90: LGTM! Proper SQL dump execution with error handling.The
runSQLDumpfunction correctly reads and executes the SQL dump file with appropriate error handling and wrapping.servers/assessment/assessments/scoreLevel/service_test.go (1)
22-34: LGTM! Proper test setup with singleton initialization.The setup correctly initializes the test database and sets the service singleton, which is essential for the service functions to work properly.
servers/assessment/categories/service_test.go (4)
15-35: Well-structured test suite setup.The test suite follows good practices with proper database setup, error handling, and cleanup. The use of
testutils.SetupTestDBprovides a clean testing environment with seeded data.
43-155: Comprehensive test coverage with good practices.The test methods cover all CRUD operations effectively:
- Tests both success and error scenarios
- Uses appropriate assertions for data validation
- Includes edge cases like non-existent entities
- Follows a logical flow (create → verify → delete → verify deletion)
145-149:✅ Verification successful
Verify the expected behavior for deleting non-existent categories.
Similar to the update test, this expects no error when deleting a non-existent category. Please confirm this idempotent behavior aligns with the service's design.
🏁 Script executed:
#!/bin/bash # Description: Check the DeleteCategory implementation to verify error handling for non-existent categories # Expected: Find the DeleteCategory function implementation ast-grep --pattern $'func DeleteCategory($_, $_) $_ { $$$ }'Length of output: 674
DeleteCategory is intentionally idempotent (no-op on missing records)
The
DeleteCategoryimplementation inservers/assessment/categories/service.go(lines 84–91) usessqlc’s Exec under the hood, which does not return an error when zero rows are affected. The existing test inservice_test.gocorrectly verifies that deleting a non-existent category returns no error. No changes required.
109-118:❓ Verification inconclusive
Verify the expected behavior for updating non-existent categories.
The test expects no error when updating a non-existent category. This suggests idempotent behavior, but please verify this is the intended business logic.
🏁 Script executed:
#!/bin/bash # Description: Check the UpdateCategory implementation to verify error handling for non-existent categories # Expected: Find the UpdateCategory function implementation ast-grep --pattern $'func UpdateCategory($_, $_, $_) $_ { $$$ }'Length of output: 1099
Confirm idempotent update behavior
TheUpdateCategoryimplementation inservers/assessment/categories/service.go(lines 70–82) uses an SQLUPDATEviaExec, which does not error when no rows match the given ID. As a result, callingUpdateCategoryon a non‐existent category will always returnnil, matching the test’s expectation.Please verify that this “no‐error if not found” behavior is the intended business logic.
- If you intend to treat a missing category as an error, modify
UpdateCategoryto checkResult.RowsAffected()and return a “not found” error when it’s zero.- Otherwise, the current implementation and test are aligned.
servers/assessment/assessments/scoreLevel/router_test.go (2)
23-52: Excellent router test setup with proper authentication mocking.The test suite setup is well-designed:
- Proper database initialization with SQL dumps
- Correct service singleton assignment
- Mock authentication middleware integration
- Clean router configuration
60-110: Comprehensive HTTP endpoint testing.The test methods provide excellent coverage:
- Tests successful data retrieval with proper JSON unmarshalling
- Validates response structure and data integrity
- Tests error scenarios with invalid UUIDs
- Covers both path parameter validation (phase ID and participation ID)
The hardcoded UUIDs are appropriate as they correspond to seeded test data.
.github/workflows/test-servers.yml (1)
8-75: Excellent selective testing workflow design.This workflow implements several best practices:
- Efficient change detection with path filters
- Matrix strategy for testing multiple server directories
- Proper Go setup with dependency caching
- Test result artifacts and annotations
- Testcontainers Ryuk disabled for CI environment
The workflow significantly improves CI efficiency by testing only changed components.
🧰 Tools
🪛 actionlint (1.7.7)
16-16: the runner of "dorny/paths-filter@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
servers/assessment/assessments/assessmentCompletion/router_test.go (2)
20-49: Well-organized test suite with proper setup.The test suite follows the established pattern with:
- Clean database setup using SQL dumps
- Proper service initialization and singleton assignment
- Mock authentication middleware integration
- Correct router configuration
57-134: Comprehensive HTTP endpoint testing with excellent error handling coverage.The test methods provide thorough coverage of various scenarios:
- Invalid JSON payload handling (400 Bad Request)
- Service error simulation (500 Internal Server Error)
- Not found scenarios with proper error responses
- Invalid UUID parameter validation for both phase and participation IDs
- DELETE operations for both existing and non-existing entities
The test assertions correctly validate HTTP status codes and the tests cover both success and failure paths effectively.
servers/assessment/categories/router_test.go (3)
19-25: LGTM! Well-structured test suite setup.The test suite follows good practices with proper setup and cleanup, dependency injection, and realistic middleware configuration.
Also applies to: 27-45
98-98:✅ Verification successful
Verify the hardcoded UUID exists in test data.
The hardcoded UUID
"25f1c984-ba31-4cf2-aa8e-5662721bf44e"in TestUpdateCategory should exist in the test database to ensure the test passes reliably.
🏁 Script executed:
#!/bin/bash # Description: Verify the hardcoded category UUID exists in test data # Expected: Find this UUID in the categories SQL dump rg "25f1c984-ba31-4cf2-aa8e-5662721bf44e" servers/assessment/database_dumps/categories.sqlLength of output: 1428
UUID present in test fixtures
The hardcoded UUID
25f1c984-ba31-4cf2-aa8e-5662721bf44eis indeed defined inservers/assessment/database_dumps/categories.sqlvia theINSERT INTO public.categorystatement, so the test data covers this case and no further action is needed.
42-42:✅ Verification successful
Verify the hardcoded email exists in test data.
The hardcoded email
"[email protected]"in the mock middleware should correspond to actual test data to ensure realistic testing scenarios.
🏁 Script executed:
#!/bin/bash # Description: Verify the hardcoded email exists in the test database dumps # Expected: Find references to this email in SQL dump files rg -i "[email protected]" servers/assessment/database_dumps/Length of output: 70
🏁 Script executed:
#!/bin/bash # Description: Search for the hardcoded email across the entire repository to locate any test fixtures, seeds, or migrations rg -i "[email protected]"Length of output: 1280
Hardcoded email exists in test data
The user email
"[email protected]"is already seeded in the test fixtures:
- servers/core/database_dumps/application_administration.sql (VALUES (‘…’, ‘[email protected]’, …))
No changes required here.
servers/assessment/assessments/assessmentCompletion/service_test.go (3)
14-19: LGTM! Clean service test structure.The test suite follows good practices with proper setup, dependency injection, and singleton pattern usage for testing.
Also applies to: 21-34
42-77: Excellent edge case coverage.The tests appropriately cover important edge cases like checking non-existent completions, unmarking non-existent entities, and handling empty result sets.
43-44:✅ Verification successful
Verify hardcoded UUIDs exist in test data.
The test uses several hardcoded UUIDs that should correspond to actual test data in the SQL dump to ensure reliable test execution.
Also applies to: 51-52, 59-60, 66-66, 73-74
🏁 Script executed:
#!/bin/bash # Description: Verify the hardcoded UUIDs exist in assessment completion test data # Expected: Find these UUIDs in the SQL dump files echo "Checking for course phase ID: 24461b6b-3c3a-4bc6-ba42-69eeb1514da9" rg "24461b6b-3c3a-4bc6-ba42-69eeb1514da9" servers/assessment/database_dumps/ echo "Checking for participation ID: 319f28d4-8877-400e-9450-d49077aae7fe" rg "319f28d4-8877-400e-9450-d49077aae7fe" servers/assessment/database_dumps/Length of output: 35076
Hardcoded UUIDs Verified in SQL Dumps
I confirmed that the UUIDs used in your service tests (
24461b6b-3c3a-4bc6-ba42-69eeb1514da9and319f28d4-8877-400e-9450-d49077aae7feon lines 43–44) appear in both
servers/assessment/database_dumps/assessmentCompletions.sqlservers/assessment/database_dumps/assessments.sqlAll other hardcoded UUIDs in your test suite follow the same pattern and are backed by entries in the SQL dumps. No changes needed.
servers/assessment/competencies/service_test.go (4)
106-142: Excellent comprehensive create test.This test thoroughly validates all aspects of competency creation including data integrity verification by listing and comparing all fields.
144-202: Outstanding update test with full lifecycle validation.The test properly creates, updates, and verifies the competency through the complete update lifecycle with thorough field validation.
272-288: Good foreign key constraint testing.Testing competency creation with an invalid category ID ensures proper database constraint validation.
63-63:✅ Verification successful
Verify hardcoded UUIDs exist in test data.
The tests use several hardcoded category and competency UUIDs that should correspond to actual test data for reliable execution.
Also applies to: 85-85, 108-108, 146-146, 206-206, 225-225
🏁 Script executed:
#!/bin/bash # Description: Verify hardcoded UUIDs exist in competency test data # Expected: Find these UUIDs in the competencies SQL dump echo "Checking competency ID: 20725c05-bfd7-45a7-a981-d092e14f98d3" rg "20725c05-bfd7-45a7-a981-d092e14f98d3" servers/assessment/database_dumps/ echo "Checking Version Control category: 25f1c984-ba31-4cf2-aa8e-5662721bf44e" rg "25f1c984-ba31-4cf2-aa8e-5662721bf44e" servers/assessment/database_dumps/ echo "Checking User Interface category: 815b159b-cab3-49b4-8060-c4722d59241d" rg "815b159b-cab3-49b4-8060-c4722d59241d" servers/assessment/database_dumps/ echo "Checking Fundamentals category: 9107c0aa-15b7-4967-bf62-6fa131f08bee" rg "9107c0aa-15b7-4967-bf62-6fa131f08bee" servers/assessment/database_dumps/Length of output: 29190
🏁 Script executed:
#!/bin/bash # Extract all hardcoded UUIDs from service_test.go and verify they exist in the SQL dumps set -e file="servers/assessment/competencies/service_test.go" echo "Extracting UUIDs from $file" uuids=$(grep -oP 'uuid\.MustParse\("\K[0-9a-fA-F-]{36}(?="\)' "$file" | sort -u) for id in $uuids; do echo echo "Verifying UUID: $id" if rg "$id" servers/assessment/database_dumps/; then echo "→ OK: $id found in dumps" else echo "→ ERROR: $id NOT found in dumps" fi doneLength of output: 371
🏁 Script executed:
#!/bin/bash # Correctly extract all UUIDs passed to uuid.MustParse() in service_test.go and verify each against the SQL dumps set -e file="servers/assessment/competencies/service_test.go" echo "Extracting UUIDs from $file…" uuids=$(grep -oP 'uuid\.MustParse\("[0-9a-fA-F-]{36}"\)' "$file" \ | sed -E 's/uuid\.MustParse\("([0-9a-fA-F-]{36})"\)/\1/' \ | sort -u) if [[ -z "$uuids" ]]; then echo "→ No UUIDs found in $file" exit 1 fi for id in $uuids; do echo echo "Verifying UUID: $id" if rg -q "$id" servers/assessment/database_dumps/; then echo "→ OK: $id found in database_dumps/" else echo "→ ERROR: $id NOT found in database_dumps/" fi doneLength of output: 2168
All hardcoded UUIDs in service_test.go are present in the SQL dumps
I’ve extracted every
uuid.MustParse(...)call and confirmed each ID—20725c05-bfd7-45a7-a981-d092e14f98d3, 25f1c984-ba31-4cf2-aa8e-5662721bf44e, 815b159b-cab3-49b4-8060-c4722d59241d, and 9107c0aa-15b7-4967-bf62-6fa131f08bee—appears in theservers/assessment/database_dumps/SQL files. No missing UUIDs were found; no changes needed here.servers/assessment/assessments/service_test.go (4)
78-85: Good use of random UUID for non-existent category test.Using
uuid.New()for testing with a non-existent category is appropriate as it tests the behavior when no data exists, which is the expected outcome.
14-19: LGTM! Clean and focused service test setup.The test suite structure follows good practices with proper database setup and cleanup.
Also applies to: 21-34
56-60: Good error case coverage.The tests properly validate error conditions for non-existent entities, ensuring robust error handling.
Also applies to: 87-91
43-43:✅ Verification successful
Verify hardcoded UUIDs exist in test data.
The tests use several hardcoded UUIDs that should correspond to actual test data in the assessments SQL dump.
Also applies to: 50-50, 63-64, 71-72
🏁 Script executed:
#!/bin/bash # Description: Verify hardcoded UUIDs exist in assessment test data # Expected: Find these UUIDs in the assessments SQL dump echo "Checking course phase ID: 24461b6b-3c3a-4bc6-ba42-69eeb1514da9" rg "24461b6b-3c3a-4bc6-ba42-69eeb1514da9" servers/assessment/database_dumps/ echo "Checking assessment ID: 1950fdb7-d736-4fe6-81f9-b8b1cf7c85df" rg "1950fdb7-d736-4fe6-81f9-b8b1cf7c85df" servers/assessment/database_dumps/ echo "Checking participation ID: ca42e447-60f9-4fe0-b297-2dae3f924fd7" rg "ca42e447-60f9-4fe0-b297-2dae3f924fd7" servers/assessment/database_dumps/ echo "Checking competency ID: eb36bf49-87c2-429b-a87e-a930630a3fe3" rg "eb36bf49-87c2-429b-a87e-a930630a3fe3" servers/assessment/database_dumps/Length of output: 45802
All hardcoded UUIDs validated against test data
Each of the hardcoded UUIDs in
service_test.go—
- Phase ID: 24461b6b-3c3a-4bc6-ba42-69eeb1514da9
- Assessment ID: 1950fdb7-d736-4fe6-81f9-b8b1cf7c85df
- Participation ID: ca42e447-60f9-4fe0-b297-2dae3f924fd7
- Competency ID: eb36bf49-87c2-429b-a87e-a930630a3fe3
—were all found in the SQL dumps under
servers/assessment/database_dumps. No changes required.servers/assessment/competencies/router_test.go (2)
1-359: Excellent test coverage with comprehensive CRUD operations.The test suite provides thorough coverage of all competency router endpoints including:
- CRUD operations (Create, Read, Update, Delete)
- Error handling for invalid UUIDs and malformed JSON
- Category-specific competency listing
- Proper HTTP status code verification
The use of testify suite pattern with proper setup/teardown is a best practice.
71-71:✅ Verification successful
Verify that hard-coded UUIDs exist in the test database.
The tests use hard-coded UUIDs (e.g.,
25f1c984-ba31-4cf2-aa8e-5662721bf44e) that must exist in the database dump for the tests to pass.Also applies to: 163-163
🏁 Script executed:
#!/bin/bash # Description: Verify that the hard-coded UUIDs used in tests exist in the database dumps # Expected: UUIDs should be found in the competencies.sql dump echo "Checking for UUID 25f1c984-ba31-4cf2-aa8e-5662721bf44e in database dumps:" rg "25f1c984-ba31-4cf2-aa8e-5662721bf44e" servers/assessment/database_dumps/ echo -e "\nChecking for other hard-coded UUIDs in test files:" rg -A 2 -B 2 "uuid\.MustParse" servers/assessment/competencies/router_test.goLength of output: 7400
All hard-coded UUIDs are present in the database dumps
Verified that the UUID
25f1c984-ba31-4cf2-aa8e-5662721bf44eused in servers/assessment/competencies/router_test.go (in TestGetCompetency, TestCreateCompetency, TestUpdateCompetency, and TestDeleteCompetency) appears in all relevant SQL dumps under servers/assessment/database_dumps/ (competencies.sql, categories.sql, assessments.sql, assessmentCompletions.sql). No further changes needed.servers/assessment/database_dumps/competencies.sql (3)
40-45: Well-designed enum type for score levels.The
score_levelenum provides a clear, type-safe way to represent competency assessment levels with meaningful progression from 'novice' to 'expert'.
158-159: Excellent constraint design for data integrity.The schema includes proper constraints:
- Unique constraint on
(course_participation_id, course_phase_id, competency_id)prevents duplicate assessments- Foreign key constraints with
CASCADE DELETEensure referential integrity- Primary key constraints on all tables
This design prevents data inconsistencies and maintains referential integrity.
Also applies to: 213-214
121-191: Comprehensive seed data for testing.The competency data includes:
- Detailed descriptions for each skill level (novice through expert)
- Realistic competencies covering Git, UI design, and software engineering
- Proper category associations
- Weighted scoring support
This provides excellent test data coverage for the assessment system.
servers/assessment/database_dumps/assessments.sql (2)
153-160: Efficient view design for completed assessments.The
completed_score_levelsview efficiently filters weighted scores to only include participants who have completed their assessments by using an EXISTS clause. This is a good performance optimization.
177-261: Extensive test data provides excellent coverage.The assessment data includes:
- Multiple participants across different course phases
- Diverse score distributions across all competency levels
- Realistic timestamps and author information
- Good coverage of all competencies and score levels
This comprehensive dataset will enable thorough testing of the scoring and completion logic.
servers/assessment/database_dumps/categories.sql (1)
176-191: Consistent category and competency data across dumps.The category and competency seed data is identical across the dumps, which is good for consistency. The categories cover the main domains (Version Control, UI, Software Engineering) with appropriate weightings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
servers/assessment/testutils/db_setup.go (1)
90-97: Consider accepting context parameter for better context propagation.The
runSQLDumpfunction usescontext.Background()instead of accepting a context parameter, which breaks the context chain from the caller.-func runSQLDump(conn *pgxpool.Pool, path string) error { +func runSQLDump(ctx context.Context, conn *pgxpool.Pool, path string) error { dump, err := os.ReadFile(path) if err != nil { return fmt.Errorf("could not read SQL dump file: %w", err) } - _, err = conn.Exec(context.Background(), string(dump)) + _, err = conn.Exec(ctx, string(dump)) return err }And update the call site:
- if err := runSQLDump(conn, sqlDumpPath); err != nil { + if err := runSQLDump(ctx, conn, sqlDumpPath); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/test-servers.yml(1 hunks)servers/assessment/assessments/scoreLevel/service_test.go(1 hunks)servers/assessment/competencies/router_test.go(1 hunks)servers/assessment/competencies/service_test.go(1 hunks)servers/assessment/database_dumps/assessmentCompletions.sql(1 hunks)servers/assessment/database_dumps/assessments.sql(1 hunks)servers/assessment/database_dumps/categories.sql(1 hunks)servers/assessment/testutils/db_setup.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- servers/assessment/database_dumps/categories.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/test-servers.yml
- servers/assessment/competencies/router_test.go
- servers/assessment/competencies/service_test.go
- servers/assessment/assessments/scoreLevel/service_test.go
- servers/assessment/database_dumps/assessmentCompletions.sql
🧰 Additional context used
🧠 Learnings (1)
servers/assessment/database_dumps/assessments.sql (1)
Learnt from: rappm
PR: ls1intum/prompt2#555
File: servers/assessment/database_dumps/assessments.sql:113-146
Timestamp: 2025-05-28T19:42:01.353Z
Learning: Database dump files used for testing don't need extensive comments or documentation. Documentation for business logic like scoring algorithms should be placed in the actual application code that uses the database views, not in the database dump files themselves.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-servers / self-team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-self-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
🔇 Additional comments (2)
servers/assessment/database_dumps/assessments.sql (1)
1-1802: LGTM! Comprehensive database dump with well-structured schema and test data.The database dump provides a complete assessment system foundation with proper constraints, indexes, views, and extensive seed data for testing purposes.
servers/assessment/testutils/db_setup.go (1)
49-58: LGTM! Error handling has been properly implemented.The previous issue with missing error handling for
container.Host()andcontainer.MappedPort()has been correctly addressed with proper error checking and container cleanup on failure.
|
LGTM |
…r database connection
✨ What is the change?
Add tests to the assessment server, rework the Go Test Workflow so it only tests the servers affected by this change.
depends on #563
📌 Reason for the change / Link to issue
closes #440
closes #541
🧪 How to Test
server/assessmentdirectorygo test -v ./...✅ PR Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
.DS_Storeto.gitignoreto prevent accidental commits of system files.Refactor
Documentation