Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Context generator #14

Merged
merged 14 commits into from
May 7, 2017
Merged

Context generator #14

merged 14 commits into from
May 7, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented Feb 23, 2017

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.

@djbe djbe force-pushed the feature/generate-contexts branch from f6ea4b1 to f210dbc Compare March 5, 2017 17:06
@djbe djbe requested a review from AliSoftware March 27, 2017 09:21
@djbe
Copy link
Member Author

djbe commented Mar 27, 2017

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)? 😆

@AliSoftware
Copy link
Contributor

AliSoftware commented Mar 27, 2017

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 #if GENERATE_CONTEXTS?

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)

@djbe djbe force-pushed the feature/generate-contexts branch 2 times, most recently from 915ea6d to f189e2d Compare March 27, 2017 18:43
@djbe
Copy link
Member Author

djbe commented Mar 27, 2017

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" {
Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was a mistake 😊

@djbe djbe mentioned this pull request Apr 2, 2017
18 tasks
@djbe djbe force-pushed the feature/generate-contexts branch from b22467f to a0d77f8 Compare April 12, 2017 13:43
@djbe
Copy link
Member Author

djbe commented Apr 24, 2017

Follow up on our discussion from a while ago:

  • Launch arguments didn't work for me. xcodebuild calls xctest with the following arguments, no matter what I tried from command line (nothing worked):
    ["/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Xcode/Agents/xctest", "-NSTreatUnknownArgumentsAsOpen", "NO", "-ApplePersistenceIgnoreState", "YES"]
  • We could just do env. variables from rake, but then users wouldn't be able to generate contexts from Xcode.

Thoughts? I think we might as well merge this.

@djbe djbe added this to the 1.1.0 milestone May 7, 2017
Copy link
Contributor

@AliSoftware AliSoftware left a 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!

@AliSoftware AliSoftware merged commit c0a5a07 into master May 7, 2017
@AliSoftware AliSoftware deleted the feature/generate-contexts branch May 7, 2017 21:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants