[NOCDX] Fix tests - #274
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the test suite by ensuring OHHTTPStubs are properly cleaned up between tests and correcting parameters and answer ordering in the quiz integration tests. Previously, leaked stubs and mismatched quiz parameters likely caused intermittent or incorrect test results.
Changes:
- Add
OHHTTPStubs.removeAllStubs()intearDown()across all worker test classes, adding the import and (where needed) thetearDown()method itself. - Fix
ConstructorIOQuizIntegrationTeststo passquizVersionID/quizSessionIDand correct the answer ordering so it matches the quiz question types (single/multi/cover/open). - Fix the regex in
ConstructorIOQuizTests.testQuizResults_WithValidRequest...to use the URL-encoded2,%203matching the actual encoded request.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| AutocompleteClientTests/FW/Logic/Worker/SearchTests.swift | Adds OHHTTPStubs import; replaces placeholder comment in tearDown with removeAllStubs(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOUserIDTests.swift | Adds OHHTTPStubs import and a new tearDown() that calls removeAllStubs(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOSearchTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIORecommendationsTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOQuizTests.swift | Adds OHHTTPStubs cleanup; updates regex to expect URL-encoded answer 2,%203. |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOQuizIntegrationTests.swift | Passes quizVersionID/quizSessionID to queries and reorders answers (seen before true) to match question types. |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOBrowseTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOBrowseItemsTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOBrowseGroupsTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOBrowseFacetsTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOBrowseFacetOptionsTests.swift | Adds OHHTTPStubs cleanup in tearDown(). |
| AutocompleteClientTests/FW/Logic/Worker/ConstructorIOAutocompleteTests.swift | Adds OHHTTPStubs import and a new tearDown() that calls removeAllStubs(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR fixes test isolation issues by adding OHHTTPStubs.removeAllStubs() in tearDown() across test files, and corrects quiz integration tests to include quizVersionID/quizSessionID parameters and fix answer ordering and URL encoding.
Inline comments: 5 discussions added
Overall Assessment:
| import OHHTTPStubs | ||
| import XCTest | ||
|
|
||
| class ConstructorIOSearchTests: XCTestCase { |
There was a problem hiding this comment.
Critical Issue: SearchTests.swift declares class ConstructorIOSearchTests: XCTestCase, which is the exact same class name already defined in ConstructorIOSearchTests.swift. Swift does not allow two types with the same name in the same module — this will cause a compiler error (invalid redeclaration of 'ConstructorIOSearchTests'). One of these files needs to be renamed (e.g., rename the class in SearchTests.swift to something like ConstructorIOSearchTestsAlt or SearchTests) or the duplicate file should be removed if it is truly redundant.
|
|
||
| let dataToReturn = TestResource.load(name: TestResource.Response.quizResultsJSONFilename) | ||
| stub(regex("https://quizzes.cnstrc.com/v1/quizzes/test-quiz/results?_dt=\(kRegexTimestamp)&a=2&a=2,3&c=\(kRegexVersion)&i=\(kRegexClientID)&key=ZqXaOfXuBWD4s3XzCI1q&s=\(kRegexSession)&\(TestConstants.defaultSegments)"), http(200, data: dataToReturn)) | ||
| stub(regex("https://quizzes.cnstrc.com/v1/quizzes/test-quiz/results?_dt=\(kRegexTimestamp)&a=2&a=2,%203&c=\(kRegexVersion)&i=\(kRegexClientID)&key=ZqXaOfXuBWD4s3XzCI1q&s=\(kRegexSession)&\(TestConstants.defaultSegments)"), http(200, data: dataToReturn)) |
There was a problem hiding this comment.
Suggestion: The URL-encoding fix ("2, 3" → "2,%203") is correct — spaces in query parameter values are percent-encoded as %20. However, it's worth noting that testQuizResults_WithValidRequest_ReturnsNonNilResponse (line 68–81) and testQuizResults_WithValidRequest_ReturnsNonNilResponseWithRequestObject (line 83–98) both use the same answer ["2, 3"] with the same stub URL, but neither test passes quizVersionID/quizSessionID. This inconsistency means these two tests exercise a different code path (no session/version) than the fixed test testQuizResults_WithValidRequest_ReturnsNonNilResponseWithRequestObject. Verify this is intentional — if the stub-based unit tests should always include session/version IDs to mirror realistic usage, both tests should also include those parameters (similar to testQuizResults_CreatesValidRequest).
| let constructorClient = ConstructorIO(config: ConstructorIOConfig(apiKey: unitTestKey)) | ||
| let expectation = XCTestExpectation(description: "Request 200") | ||
| let query = CIOQuizQuery(quizID: "test-quiz", answers: [["1"], ["1", "2"], ["true"], ["seen"]]) | ||
| let query = CIOQuizQuery(quizID: "test-quiz", answers: [["1"], ["1", "2"], ["seen"], ["true"]], quizVersionID: self.quizVersionID, quizSessionID: self.quizSessionID) |
There was a problem hiding this comment.
Suggestion: After this fix, testGetQuizResults and testGetQuizResults_WithVersionIDAndSessionID now have identical query construction — same quizID, same answers, same quizVersionID, and same quizSessionID. The only difference in their assertions is that testGetQuizResults_WithVersionIDAndSessionID additionally checks XCTAssertEqual(responseData.quizVersionID, self.quizVersionID) and XCTAssertEqual(responseData.quizSessionID, self.quizSessionID). Consider either merging these two tests into one comprehensive test (asserting both the general result data and the specific version/session IDs) to reduce duplication, or add a comment explaining why they must remain separate.
| let constructorClient = ConstructorIO(config: ConstructorIOConfig(apiKey: unitTestKey)) | ||
| let expectation = XCTestExpectation(description: "Request 200") | ||
| let query = CIOQuizQuery(quizID: "test-quiz", answers: [["1"]]) | ||
| let query = CIOQuizQuery(quizID: "test-quiz", answers: [["1"]], quizVersionID: self.quizVersionID, quizSessionID: self.quizSessionID) |
There was a problem hiding this comment.
Suggestion: Now that all testGetQuizNextQuestion_With* tests pass quizVersionID and quizSessionID, they all depend on the specific quiz state tied to quizVersionID = "cc8a9dac-d337-4dbc-bbf7-48469c0de7af". The pre-existing test testGetQuizNextQuestion (line 49) and testGetQuizNextQuestion_WithVersionIDAndSessionID (line 83) send requests without and with those IDs respectively and both expect nextQuestion.id == 1. The newly fixed tests start with 1–3 answers and expect subsequent question IDs (2, 3, 4). This is a meaningful progression and the fix looks correct, but it would benefit from a short comment on the class explaining the expected quiz state machine (question sequence) assumed by the server-side test fixture, to help future maintainers understand why the answer orderings matter.
Title
Fix tests by properly cleaning up HTTP stubs and correcting quiz integration tests.
Summary
OHHTTPStubs.removeAllStubs()intearDown()across all test files to prevent stub leakage between teststearDown()methods inConstructorIOAutocompleteTestsandConstructorIOUserIDTestsquizVersionIDandquizSessionIDparameters and correct answer ordering"2, 3"→"2,%203")