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

Introduce test_specification DSL #369

Merged
merged 1 commit into from
Apr 24, 2017
Merged

Conversation

dnkoutso
Copy link
Contributor

@dnkoutso dnkoutso commented Mar 25, 2017

I'd like to re-open the "project" for adding test support to CocoaPods.

We have 100s of projects that we typically end up having to include a <ProjectName>Tests.xcodeproj that includes all of our test sources and a Podfile that integrates it with the pod locally.

I've chatted with @orta and @benasher44 on bringing this back. It would be awesome for podspecs to become more self-contained and when consumed as development pods the Pods.xcodeproj can create a test bundle target to include test sources so you can run them.

We are also heavy users of development pods in our main project causing us to create multiple xcodeproj just to support our tests for pods we will never publish which also complicates CI further.

This is the first PR that brings back the DSL to include a test_spec.

We won't be going super advanced on this to begin with. Test specs should create a test bundle target in the Pods.xcodeproj and include the test sources. The validator should also be updated to run the tests if a test_spec is present before publishing a pod. If no test_spec is specified everything should continue to work the way it is today.

@dnkoutso dnkoutso force-pushed the test_spec_dsl branch 2 times, most recently from 33f0b21 to 448f753 Compare March 25, 2017 05:34
* None.
* Introduce `test_specification` DSL
[Dimitris Koutsogiorgas](https://github.com/dnkoutso)
[Kyle Fuller](https://github.com/kylef)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the work was done so I included the credit.

# end
#
def test_spec(&block)
subspec = Specification.new(self, 'Tests', true, &block)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benasher44 do you think we should force the name to always be 'Tests' or let podspec authors to configure it? This means that each spec (including root) can have 1 and only 1 test spec right?

Copy link
Member

Choose a reason for hiding this comment

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

To avoid future DSL gotchas: it's worth considering about making sure a test target can be configured to support multiple test targets, for example you might have unit tests + uiintegration tests (we do in Eigen, but we don't in any of our libs FWIW)

You might not have to do the work today, but building the DSL in a way that supports that is 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orta good point. Having a test spec is one thing but there is no way to configure it to generate a UI test target or a unit test target. Do those differ much?

Copy link
Member

Choose a reason for hiding this comment

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

They differ considerably in implementation (one is a bundle injected into an app target, the other an app target)

@dnkoutso
Copy link
Contributor Author

Additional cool thing could easily be a small plugin command that generates the xcodeproj for developers.

This means a developer can clone a repo and through the podspec generate the xcodeproj to develop on the pod. This means no more xcodeproj checked in to the repo which means no more ugly conflicts.

# @param [Bool] all_platforms
# whether the dependencies should be returned for all platforms
# instead of the active one.
# @param [Platform] platform
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed documentation for this method.

Copy link
Member

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

This is a really good start, excited for this to land 👍

# end
# end
#
def test_spec(&block)
Copy link
Member

Choose a reason for hiding this comment

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

This should maybe be:

def test_spec(name = 'Tests', type = :unit, &block)

or similar to allow for extension into different test target types in the future.

@dnkoutso
Copy link
Contributor Author

Will rebase and resume this a bit this weekend. still high on my radar for 1.3.0.

@dnkoutso dnkoutso force-pushed the test_spec_dsl branch 4 times, most recently from 278c88b to ff2bf56 Compare April 21, 2017 01:50
@dnkoutso
Copy link
Contributor Author

@orta @dantoml getting closer can you take another look?

@spec = Spec.new do |spec|
spec.name = 'Spec'
spec.test_spec do |test_spec|
test_spec.test_type = :unit
Copy link
Contributor Author

@dnkoutso dnkoutso Apr 21, 2017

Choose a reason for hiding this comment

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

@orta @dantoml if I a podspec author does not call this, how can I enforce a default value? Is it on the Specification constructor that I add test_spec = :unit if test_specification or something akin to that?

Copy link
Contributor Author

@dnkoutso dnkoutso Apr 21, 2017

Choose a reason for hiding this comment

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

added it in the initializer of Specification.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it now as the attribute now has a default_value set when used with a consumer.rb.

@orta
Copy link
Member

orta commented Apr 21, 2017

This looks like what I'd expect it to look like 👍

@dnkoutso dnkoutso force-pushed the test_spec_dsl branch 3 times, most recently from 9d7a63c to fab2c45 Compare April 21, 2017 22:09
@dnkoutso
Copy link
Contributor Author

I think its ready!

@dnkoutso dnkoutso force-pushed the test_spec_dsl branch 4 times, most recently from 5e55185 to 08dc711 Compare April 22, 2017 00:21
#
# @param [Symbol] type
# The test type to use.
attribute :test_type,
Copy link
Contributor Author

@dnkoutso dnkoutso Apr 22, 2017

Choose a reason for hiding this comment

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

@orta @dantoml I am reading the code more I dont think :keys is what we want here so I removed it. Keys is meant to be used to validate the keys within a hash value, not for limiting the potential values you can have for this attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead the Linter takes care of validating the value is correct, which I added a test for.

@dnkoutso dnkoutso force-pushed the test_spec_dsl branch 6 times, most recently from cc50e76 to 6ecbf89 Compare April 22, 2017 01:44
@dnkoutso
Copy link
Contributor Author

@orta @dantoml alright I think its finally ready.

# @param [Symbol] type
# The test type to use.
attribute :test_type,
:default_value => :unit,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this exists!

@dnkoutso
Copy link
Contributor Author

Very excited for this

@orta
Copy link
Member

orta commented Apr 23, 2017

👍

@dnkoutso
Copy link
Contributor Author

@dantoml @orta with your blessing I can merge.

Copy link
Member

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

:shipit:

@dnkoutso dnkoutso merged commit 75fd14f into CocoaPods:master Apr 24, 2017
@dnkoutso
Copy link
Contributor Author

Merging! Lets this party started.

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.

4 participants