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

Honor test spec deployment target during validation. #9999

Merged

Conversation

dnkoutso
Copy link
Contributor

If a test spec has a different deployment target than the library spec during validation this was not honored by the validator.

@dnkoutso dnkoutso force-pushed the fix_deployment_target_test_spec branch from beb630c to f6dac8d Compare August 20, 2020 22:52
@@ -757,7 +759,7 @@ def test_pod
UI.warn "Skipping test spec `#{test_spec.name}` on platform `#{consumer.platform_name}` since it is not supported.\n".yellow
else
scheme = @installer.target_installation_results.first[pod_target.name].native_target_for_spec(test_spec)
output = xcodebuild('test', scheme, 'Debug')
output = xcodebuild('test', scheme, 'Debug', test_spec.deployment_target(consumer.platform_name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method will also use the parent spec if none is explicitly specified.

@@ -1048,7 +1050,7 @@ def parse_xcodebuild_output(output)
# @return [String] Executes xcodebuild in the current working directory and
# returns its output (both STDOUT and STDERR).
#
def xcodebuild(action, scheme, configuration)
def xcodebuild(action, scheme, configuration, deployment_target)
Copy link
Member

Choose a reason for hiding this comment

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

lol love how this is the only change to the method. variable shadowing ftw?

Copy link
Member

Choose a reason for hiding this comment

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

nit: use a named parameter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

love it

@@ -1048,7 +1050,7 @@ def parse_xcodebuild_output(output)
# @return [String] Executes xcodebuild in the current working directory and
# returns its output (both STDOUT and STDERR).
#
def xcodebuild(action, scheme, configuration)
def xcodebuild(action, scheme, configuration, deployment_target)
Copy link
Member

Choose a reason for hiding this comment

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

nit: use a named parameter ?

@@ -600,6 +600,39 @@ def podspec_path(name = 'JSONKit', version = '1.4')
validator.results_message.strip.should.be.empty
end

it 'uses the deployment target of the current test spec' do
Copy link
Member

Choose a reason for hiding this comment

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

it's not super clear to me what in here is validating that the correct deployment target is being used for xcodebuild ?

Copy link
Member

Choose a reason for hiding this comment

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

it pulls a destination from Fourflusher once requesting iOS 8 for building the library and once requesting iOS 9 to run the test. at least that was my understanding. maybe would be better to add an expectation that xcodebuild is provided the correct deployment targets instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap. Those tests are super convoluted and hard to follow. I verified without my change this test fails.

@dnkoutso dnkoutso force-pushed the fix_deployment_target_test_spec branch 3 times, most recently from 80335dc to 81e9d05 Compare August 20, 2020 23:10
@dnkoutso dnkoutso force-pushed the fix_deployment_target_test_spec branch from 81e9d05 to a6f9bfc Compare August 20, 2020 23:22
@dnkoutso dnkoutso merged commit 1b3e78a into CocoaPods:1-10-stable Aug 20, 2020
@dnkoutso dnkoutso deleted the fix_deployment_target_test_spec branch August 20, 2020 23:42
@@ -1048,7 +1050,7 @@ def parse_xcodebuild_output(output)
# @return [String] Executes xcodebuild in the current working directory and
# returns its output (both STDOUT and STDERR).
#
def xcodebuild(action, scheme, configuration)
def xcodebuild(action, scheme, configuration, deployment_target:)
Copy link
Contributor

@gyfelton gyfelton Aug 21, 2020

Choose a reason for hiding this comment

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

i see this is a new param, but the implementation does not seem to use it (or where this deployment_target refer to down in line 1059/1061? @dnkoutso

@dnkoutso dnkoutso added this to the 1.10.0 milestone Sep 15, 2020
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

4 participants