Skip to content
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

Generalize test mocks #179

Merged
merged 21 commits into from
Feb 21, 2024
Merged

Generalize test mocks #179

merged 21 commits into from
Feb 21, 2024

Conversation

aokj4ck
Copy link
Contributor

@aokj4ck aokj4ck commented Feb 14, 2024

Description

Subtask of: https://mapbox.atlassian.net/browse/SSDK-557

  • Replace MockResponse enum with a MockResponse protocol
  • Add an ApiType for every MockResponse instance
  • Split previous MockResponse enum into
    1. SBSMockResponse
    2. GeocodingMockResponse
    3. AutofillMockResponse
  • Change MockWebServer and XCTestCase-base-classes to have a generic type conforming to MockResponse
    • This allows us to special test cases according to the Mock data and its corresponding ApiType
    • A test case that specializes in GeocodingMockResponse can:
      • Invoke server.setResponse(_ response: Mock) and the generic Mock will be GeocodingMockResponse
      • Query Mock.apiType to retrieve GeocodingMockResponse.apiType which will return .geocoding and instantiate a testable SearchEngine with this value
      • Always convey the ApiType that the tests will use because it's prominently shown in the generic value Mock and available in the strong type inferences.
  • Distinguish mock data by the API type so that it is aligned throughout the base class, to the mock server, into the mock data, and into every test.
  • Run UI Tests in every PR
  • All tests now use mocked data through the Swifter local HTTP server
    • Previously a variety of tests used production api.mapbox.com to query
  • Skip OfflineIntegrationTests (for now)

Checklist

  • Update CHANGELOG
  • Align UI tests by mock
  • Rename LegacyResponse to GeocodingMockResponse
  • Pare unused cases from Geocoding, SBS mocks

Screenshots

Address Autofill Integration Test
With mixed mocks With MockResponse generics
generalize-mocks-main-address-autofill generalize-mocks-generics-address-autofill
  • Non-generic base TestCase class
  • ApiType is constant in setUp function
  • setResponse(.retrieveAddressSanFrancisco) and .suggestAddressSanFrancisco are of enum type MockResponse and mixed with mocks from all other Api types (geocoding, SBS, future search-box) which can allow misalignments between api types and mock data
  • Base TestCase class has a generic that conforms to MockResponse. This guarantees that the mock data is typed according to the mock's ApiType and that we can instantiate a SearchEngine using the same ApiType dynamically.
  • The ApiType in setUp function is dynamic and matches the generic MockResponse.apiType
  • setResponse(.retrieveAddressSanFrancisco) and .suggestAddressSanFrancisco are of type AutofillMockResponse so we can use those strongly typed values (see …Address…) that only occur in the AutofillMockResponse enum so we know they are provided from this API, correct for this API, and not confused with any other API
Geocoding Integration Test
With mixed mocks With MockResponse generics
generalize-mocks-main-category generalize-mocks-generics-category
  • Non-generic base TestCase class
  • ApiType.SBS is constant in setUp function
  • setResponse(.categoryCafe) is of enum type MockResponse and mixed with mocks from all other Api types (geocoding, autofill, future search-box) which can allow misalignments between api types and mock data
  • Base TestCase class has a generic of SBSMockResponse that conforms to MockResponse. This guarantees that the mock data is typed according to the mock's ApiType and that we can instantiate a CategorySearchEngine using the same ApiType dynamically.
  • The ApiType in setUpWithError function is dynamic and matches the generic MockResponse.apiType
  • setResponse(.categoryCafe) is of type SBSMockResponse so we can use those strongly typed values that only occur in the SBSMockResponse enum so we know they are provided from this API, correct for this API, and not confused with any other API

- Rename existing MockResponse to LegacyResponse and conform to new MockResponse
- Move httpMethod and path accessors out of MockWebServer and into MockResponse instances
- Begin moving toward generalized MockResponses to distinguish Mocks by API type of geocoding, SBS, and search-box
…ntaining a MockResponse

- Conform all integration tests to generics with LegacyResponse as the value for now
- Next up: split LegacyResponse into specific enums for each API type
…tofill

- Add static MockResponse.apiType and associated type Mock in MockServerIntegrationTestCase so that subclasses can query Mock.apiType when instantiating SearchEngines based on the generic.
	- Also provides the MockResponse parameter generic type
- Add extension on CoreSearchEngine.ApiType to bridge to MapboxSearch.ApiType enum (does not contain Autofill or search-box)
- Replace hard-coded apiType: .geocoding parameters with XCTUnwrap(apiType: Mock.coreApiType.toSDKType())
- Add AutofillMockResponse with only endpoints used by the Autofill engine and tests!
@aokj4ck
Copy link
Contributor Author

aokj4ck commented Feb 14, 2024

Depends on #167 which adds new tests

@aokj4ck aokj4ck marked this pull request as ready for review February 15, 2024 14:47
@aokj4ck aokj4ck requested review from a team as code owners February 15, 2024 14:47
@aokj4ck aokj4ck requested review from kried and Udumft February 15, 2024 14:47
@aokj4ck aokj4ck requested a review from OdNairy February 15, 2024 20:19
OdNairy
OdNairy previously approved these changes Feb 16, 2024
class MockServerIntegrationTestCase: XCTestCase {
let server = MockWebServer()
class MockServerIntegrationTestCase<Mock: MockResponse>: XCTestCase {
typealias Mock = Mock
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like we can remove explicit typealias declaration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would cause the Test subclasses to need either:

  1. declare a name in the generic, such as <Mock: MockResponse> (more noise, but explicit which is nice) or
  2. change Mock usages to the default name T, such as T.coreApiType.toSDKType() (more obtuse IMHO)

The typealias is inherited and I think it's the simplest approach (although you do have to visit the parent class definition to find the typealias.) Let me know what you think.

Tests/MapboxSearchUITests/MockServerUITestCase.swift Outdated Show resolved Hide resolved
OdNairy
OdNairy previously approved these changes Feb 19, 2024
Copy link
Contributor

@OdNairy OdNairy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM overall, although I'm not an expert in iOS stuff :)

@aokj4ck aokj4ck merged commit a66e592 into main Feb 21, 2024
5 checks passed
@aokj4ck aokj4ck deleted the generalize-test-mocks branch February 21, 2024 17:06
aokj4ck added a commit that referenced this pull request Apr 5, 2024
**Subtask of:** https://mapbox.atlassian.net/browse/SSDK-557

- Replace MockResponse enum with a MockResponse protocol
- Add an ApiType for every MockResponse instance
- Split previous MockResponse enum into
    1. SBSMockResponse
    2. GeocodingMockResponse
    3. AutofillMockResponse
- Change MockWebServer and XCTestCase-base-classes to have a generic type conforming to MockResponse
    - This allows us to special test cases according to the Mock data and its corresponding ApiType
    - A test case that specializes in GeocodingMockResponse can:
        - Invoke `server.setResponse(_ response: Mock)` and the generic Mock will be GeocodingMockResponse
        - Query Mock.apiType to retrieve GeocodingMockResponse.apiType which will return .geocoding and instantiate a testable SearchEngine with this value
        - Always convey the ApiType that the tests will use because it's prominently shown in the generic value Mock and available in the strong type inferences.
- Distinguish mock data by the API type so that it is aligned throughout the base class, to the mock server, into the mock data, and into every test.
- Run UI Tests in every PR
- All tests now use mocked data through the Swifter local HTTP server
    - Previously a variety of tests used production api.mapbox.com to query
- Skip OfflineIntegrationTests (for now)

- [x] Update `CHANGELOG`
- [x] Align UI tests by mock
- [x] Rename LegacyResponse to GeocodingMockResponse
- [x] Pare unused cases from Geocoding, SBS mocks

| With mixed mocks | With MockResponse generics |
| -- | -- |
| <img width="844" alt="generalize-mocks-main-address-autofill" src="https://github.com/mapbox/mapbox-search-ios/assets/384288/e0ade188-4b7d-4850-a9da-b0b6e46ce91d"> | <img width="909" alt="generalize-mocks-generics-address-autofill" src="https://github.com/mapbox/mapbox-search-ios/assets/384288/353dcfca-dd3b-479e-8424-61bd00603371"> |
| <ul><li>Non-generic base TestCase class</li><li> ApiType is constant in setUp function</li><li> `setResponse(.retrieveAddressSanFrancisco)` and `.suggestAddressSanFrancisco` are of enum type MockResponse and mixed with mocks from all other Api types (geocoding, SBS, future search-box) which can allow misalignments between api types and mock data</li></ul> | <ul><li>Base TestCase class has a generic that conforms to MockResponse. This guarantees that the mock data is typed according to the mock's ApiType and that we can instantiate a SearchEngine using the same ApiType dynamically.</li><li>The ApiType in `setUp` function is dynamic and matches the generic MockResponse.apiType</li><li>`setResponse(.retrieveAddressSanFrancisco)` and `.suggestAddressSanFrancisco` are of type AutofillMockResponse so we can use those strongly typed values (see `…Address…`) that only occur in the AutofillMockResponse enum so we _know_ they are provided from this API, correct for this API, and not confused with any other API</li></ul> |

| With mixed mocks | With MockResponse generics |
| -- | -- |
| <img width="779" alt="generalize-mocks-main-category" src="https://github.com/mapbox/mapbox-search-ios/assets/384288/a77efd9e-ea71-4a0e-bf09-96a7769e903e"> | <img width="927" alt="generalize-mocks-generics-category" src="https://github.com/mapbox/mapbox-search-ios/assets/384288/ef125ec6-2624-4822-a501-bbe3e3d94aad"> |
| <ul><li>Non-generic base TestCase class</li><li> ApiType.SBS is constant in setUp function</li><li> `setResponse(.categoryCafe)` is of enum type MockResponse and mixed with mocks from all other Api types (geocoding, autofill, future search-box) which can allow misalignments between api types and mock data</li></ul> | <ul><li>Base TestCase class has a generic of SBSMockResponse that conforms to MockResponse. This guarantees that the mock data is typed according to the mock's ApiType and that we can instantiate a CategorySearchEngine using the same ApiType dynamically.</li><li>The ApiType in `setUpWithError` function is dynamic and matches the generic MockResponse.apiType</li><li>`setResponse(.categoryCafe)` is of type SBSMockResponse so we can use those strongly typed values that only occur in the SBSMockResponse enum so we _know_ they are provided from this API, correct for this API, and not confused with any other API</li></ul> |

Conflicts:
	CHANGELOG.md
	MapboxSearch.xcodeproj/project.pbxproj
	Tests/MapboxSearchIntegrationTests/CategorySearchEngineIntegrationTests.swift
	Tests/MapboxSearchIntegrationTests/DiscoverIntegrationTests.swift
	Tests/MapboxSearchIntegrationTests/SearchEngineGeocodingIntegrationTests.swift
	Tests/MapboxSearchIntegrationTests/SearchEngineIntegrationTests.swift
	Tests/MapboxSearchUITests/MockWebServer/MockResponse.swift
	Tests/MapboxSearchUITests/MockWebServer/MockWebServer.swift
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