-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
f6ea4b1
to
f210dbc
Compare
Need this for other PRs (so I don't have to do manually edit them all the time), so if we could get this merged soon(-ish)? 😆 |
Having a dedicated target which (if I'm not mistaken from reading that directly from GitHub) is a duplicate of the Test Target, has the risk of forgetting to add new test files when we have more tests and parsers in the future. Wouldn't it be more maintainable to have the "Generate Contexts" scheme which would build the Test target (the same as the existing scheme), so run the unit tests the same way… and use an Environment Variable or Launch Arguments to differentiate — instead of using This way we would not need a separate target that would be the exact duplicate of the test target (except for that build setting) but instead still have one unique test target, and the main scheme will run the test but the "Generate Contexts" scheme will run the exact same tests, but with an additional Launch Argument in the scheme. (We could even imagine instead to only keep one single scheme, not create a separate one, and have the Launch Argument pre-filled but unchecked, as we do for the Run action… but tbh I prefer having a separate scheme with an explicit name — as it would limit the risk of generating the contexts instead of testing them and make it more explicit for new contributors of what this action does, and that dedicated scheme can also only pilot the Test target but never be used for the App target) |
915ea6d
to
f189e2d
Compare
Using env. variables didn't work, so I settled for creating a scheme. Also created a rake task that goes along with it. |
@@ -76,16 +76,16 @@ func XCTDiffContexts(_ result: [String: Any], | |||
sub directory: Fixtures.Directory, | |||
file: StaticString = #file, | |||
line: UInt = #line) { | |||
#if GENERATE_CONTEXTS | |||
if ProcessInfo().environment["GENERATE_CONTEXTS"] == "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.
If I read the xcscheme
properly, you gave it the value Test
not the value YES
, so you should check against that Test
value not against 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.
That was a mistake 😊
… of expected context
b22467f
to
a0d77f8
Compare
Follow up on our discussion from a while ago:
Thoughts? I think we might as well merge this. |
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.
Only thing missing is some mention of this in a CONTRIBUTING.md or similar, and after that it's good to merge!
Use the "Generate Contexts" scheme to generate contexts instead of testing them. Also, had to update the pods --> use new PathKit methods to simplify some small things.