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

Add CoreAPI tests #83

Merged
merged 10 commits into from
Mar 1, 2019
Merged

Add CoreAPI tests #83

merged 10 commits into from
Mar 1, 2019

Conversation

squarefrog
Copy link
Contributor

@squarefrog squarefrog commented Feb 28, 2019

Here are some tests for CoreAPI. This tests the current behaviour, but I'd like to see this expanded in the future, as I'll mention below:

Notes

  • I had to modify the getData(forRoute:parameters:completion) method to accept a mocked session. This doesn't affect the call site, thanks to default values.

  • CoreAPI currently just returns CoreAPIError.random if there is no data. I feel this would be better returning the actual error returned by the URLSession.dataTask.

  • Additionally, there may be some merit in using the status code from the URL response to check its in the 200-299 range.

  • I modified the Makefile to accept an array of test schemes, then look through them to run the tests.

@kgellci
Copy link
Owner

kgellci commented Feb 28, 2019

Some lint issues, can run make lint to catch

* master:
  Use `createIntermediateGroups` in project.yml (kgellci#84)
  Organise CoreTests into its own folder (kgellci#81)
  Trim trailing whitespace and newlines HTML
@squarefrog
Copy link
Contributor Author

squarefrog commented Feb 28, 2019

Thats fine. I'd be inclined to ignore force_cast in tests though. I think safely unwrapping adds little benefit in a test. Normally, I'd override the .swiftlint.yml in the tests folder, but this will result in a lot of duplication across all the test targets. I suppose this can be tackled later.

On a side note, perhaps it would be prudent to setup Danger to catch things like lint issues?

@kgellci
Copy link
Owner

kgellci commented Feb 28, 2019

Feel free to add a ticket for danger. You can also disable swiftlint rules per file: https://github.com/realm/SwiftLint#disable-rules-in-code

@squarefrog
Copy link
Contributor Author

True, perhaps I’d prefer that to wrapping the code block with disable rules.

Copy link
Owner

@kgellci kgellci left a comment

Choose a reason for hiding this comment

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

👍

@kgellci kgellci merged commit 3eebeeb into kgellci:master Mar 1, 2019
@squarefrog squarefrog deleted the CoreAPITests branch March 1, 2019 09:11
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.

None yet

2 participants