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

Make pod install work on Cocoapods 1.6.0.beta.2 #5966

Merged
merged 20 commits into from Jan 11, 2019

Conversation

andrewoverton
Copy link
Contributor

@andrewoverton andrewoverton commented Dec 10, 2018

Original PR description:

On Cocoapods version 1.6.0 beta 2 we get an error on pod install because certain test targets don't have any sources to compile. This PR adds some skeleton unit test source files to get that working. Is everyone okay with these (for now) basically empty files?

Closes #5825

FInal PR description:

The original aim of this PR was to get the project working with the Cocoapods 1.6 beta.

I initially didn't know where to begin getting it to work, so I used pod lib lint to get answers. pod lib lint had issues with the following things:

  1. test_specs not having any sources to compile. This was due to our pattern of putting unit test test_specs inside of parent test_specs that didn't have anything else.

To address this I did two things. First, I moved away from the nested test_spec pattern. Now, instead of "Subspec/tests/unit_tests" it's "Subspec/UnitTests". This is more in line with the style Cocoapods uses here. Secondly, I added some dummy files for things like UIMetrics and MaterialMath.

  1. Importing headers across modules without using framework style imports.

I added framework style imports. This required some additional rewrite rules in the kokoro script.

  1. Dependencies across subspecs being implicit.

I made them explicit by adding dependency statements to the podspecs where needed.

pod lib lint didn't take issue with this, but I also saw that MaterialComponents was depending on MaterialComponentsBeta in various places. For example, all the theming extensions were in beta, but the tests for them weren't. I moved the tests to test_specs within the beta extensions. This required some directory structure changes. These changes then required some BUILD file changes. Making BUILD file changes made me realize that some swift unit tests weren't being accounted for by bazel. I took care of this too.

This PR also fixes some swift debugging stuff--"po" statements that weren't working before now do. I didn't rigorously verify this, but I also think this PR will lead to faster clean build times? It kinda seemed like it was while I was working on it.

@andrewoverton andrewoverton requested a review from a team as a code owner December 10, 2018 16:23
@material-automation
Copy link

This PR affects multiple components.

@jverkoey
Copy link
Contributor

Testing failed:
	Cannot find interface declaration for 'MaterialApplicationTests'
	Class 'MaterialApplicationTests' defined without specifying a base class
** TEST FAILED **

@material-automation
Copy link

This PR affects multiple components.

Copy link
Contributor

@jverkoey jverkoey left a comment

Choose a reason for hiding this comment

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

Empty files seems preferable over removing the test targets and potentially forgetting to add them back in the future, so that aspect LGTM.

Please add the error message this resolves to the PR description.

@andrewoverton
Copy link
Contributor Author

Let's hold off on merging this for a bit. I'm finding that with the new Cocoapods the MaterialComponents-Unit-Tests target has become a bunch of different targets, one for each component. Still figuring out the implications of this...

@andrewoverton
Copy link
Contributor Author

@jverkoey what I described in the last comment appears to be the result of CocoaPods/CocoaPods#7908. It's a feature and not a bug. Maybe we should remove the test_specs nested within the component subspecs and just have one unit test_spec that's a sibling of all the individual component subspecs. What do you think?

@jverkoey
Copy link
Contributor

jverkoey commented Dec 10, 2018

Aye we may want to reach out to cocoa pods team to give feedback on the beta.

Separate test targets is great in principle, but is less great that there’s not an easy way to run all of them from Xcode as a result of pod install. I believe we’ll need to maintain our scheme as we add/remove targets.

@andrewoverton
Copy link
Contributor Author

I did consider filing an issue just to let them know about our situation and see if they had any guidance..

Taking a step back from our specific configuration, it does kinda make sense that different test_specs declared in the podspec would end up as different targets. That's actually kind of intuitive to me :/

We could include all the new component-specific test targets in the Catalog and Dragons schemes like we currently do with the catch-all unit test target? That way we could run them from Xcode like we currently do.

@jverkoey
Copy link
Contributor

Agreed that this is intuitively more ideal. I posted a comment to that PR. In the meantime we’ll need to manage the scheme’s test targets ourselves.

@material-automation
Copy link

This PR affects multiple components.

@material-automation
Copy link

This PR affects multiple components.

… beta subspecs in non-beta components (i.e. Buttons+Theming in Buttons)
…jective-C module" errors and because it allows you to click into Swift modules instead of seeing the question mark.

Note: Using new build system requires workaround provided here: CocoaPods/CocoaPods#8105 (comment)
… that makes it work doesn't exist in Cocoapods 1.5
@material-automation
Copy link

This PR affects multiple components.

@andrewoverton
Copy link
Contributor Author

This MDCRippleLayerTests test case may be flakey, do you mind if I comment it out in this PR @yarneo?

components/Ripple/tests/unit/MDCRippleLayerTests.m:171: error: -[MDCRippleLayerTests testAnimationTimingInSpeedScaledLayer] : ((animation.beginTime) equal to (startTime) +/- (0.010)) failed: ("6142.2702562") is not equal to ("6142.32490938") +/- ("0.01")

@andrewoverton andrewoverton merged commit 056e470 into material-components:develop Jan 11, 2019
@andrewoverton andrewoverton deleted the issue-5956 branch December 8, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants