-
-
Notifications
You must be signed in to change notification settings - Fork 82
Split out IntegrationTests
into separate testTarget
#157
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
Split out IntegrationTests
into separate testTarget
#157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a very good idea to me. Looks good aside from two nits.
if let boolValue = Bool(rawValue) { return boolValue } | ||
if let intValue = Int(rawValue) { return intValue == 1 } | ||
return rawValue.lowercased() == "yes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might as well pull this out into a separate function given it's used below as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is previous code just copy and pasted here. If I try to refactor this we end up with a warning
. Would love to keep as is for now. We should refactor the perf tests to allocation tests anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me to leave this for now. Out of curiosity, why do we end up with a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This code is never reached" for debug builds.
f7d6fb3
to
b3820c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM but you haven't changed any of the CI setup - are you planning to?
This PR splits out all the integration tests into a separate test target. This makes a number of things easier in day to day development:
I think after this change we could change CI to:
Unit testing for:
Integration Testing tests for: