Skip to content

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Mar 19, 2024

Description

This will make it easier to run the tests once the fixture files move around.

If a file is misplaced, the test will fail instead of crashing.

Operationally, failing is better than crashing because it generates a
list of all misplaced tests, so that they can be addressed in one go. The alternative would be a run tests, tests crash, fix individual file path loop for as many misplaced fixture files there are.

Unfortunately, the diff is extensive. However, with the exception of RemoteTestCase.swift which changed in isolation in 007e11a, the pattern for all the changes is the same:

From:

func testSomething() {
    stub(condition: {  ... }) { _ in
      let stubPath = OHPathForFile("fixture.json", type(of: self))
      return fixture(filePath: stubPath!, ...)
    }
    ...

To:

func testSomething() throws { // test method now throws because of try XCTUnwrap
    // stubPath defined outside stub closure because the closure does not throw
    let stubPath = try XCTUnwrap(OHPathForFile("fixture.json", type(of: self))) 
    stub(condition: {  ... }) { _ in
      return fixture(filePath: stubPath, ...) // direct access instead of force unwrap
    }
    ...

Testing Details

Green CI. This is only a unit tests implementation change.

Next up

Basically, cherry-pick the work from #738 but in a neat and reviewable order:

  • Move files into Sources/ and Tests/
  • Introduce Bundle helper that differentiates between SPM and CocoaPods installations
  • Isolate core API objects in dedicated package
  • Then, new ground, define protocol abstraction to work around the Objective-C / Swift inter-op issue

  • Please check here if your pull request includes additional test coverage. — N.A.
  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary. — N.A.

mokagio added 2 commits March 19, 2024 16:40
This will make it easier to run the tests once the fixture files move
around. If a file is misplaced, the test will fail instead of crashing.
Operationally, failing is better than crashing because it generates a
list of all misplaced tests, so that they can be addressed in one go.
The alternative would be a run tests, tests crash, fix individual file
path loop for as many misplaced fixture files there are.
@mokagio mokagio requested a review from crazytonyli March 19, 2024 08:44
stub(condition: {request in request.url?.absoluteString == testURLString}) { _ in
let stubPath = OHPathForFile("empty.json", type(of: self))
return fixture(filePath: stubPath!, headers: ["Content-Type" as NSObject: "application/json" as AnyObject])
return fixture(filePath: stubPath, headers: ["Content-Type" as NSObject: "application/json" as AnyObject])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header dictionary for content type JSON is repeated all over the place.

It could be DRYed in some form. I decided to keep it out because this PR is already noisy. Besides, the focus of this stream of work is to prepare for SPM. DRYing this definition would be useful, but doesn't bring us closer to that goal.

Comment on lines +149 to +153
guard let stubPath = OHPathForFile(files[callCounter], type(of: self)) else {
XCTFail("Could not find file at path '\(files[callCounter])'.")
let error = NSError(domain: "RemoteTestCase", code: 0, userInfo: nil)
return HTTPStubsResponse(error: error)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to fail in this way, also returning an error because we're inside a closure and the file path is computed based on the callCounter value.

@mokagio mokagio merged commit 97140f0 into trunk Mar 24, 2024
@mokagio mokagio deleted the mokagio/use-xctunwrap-over-force-unwrap branch March 24, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants