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.

Sign up for GitHub

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

Jump to bottom

Consolidate Quickstart CI script #1258

Merged
merged 56 commits into from
Aug 23, 2021
Merged

Consolidate Quickstart CI script #1258

merged 56 commits into from
Aug 23, 2021

Conversation

jrcrespoh
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Aug 21, 2021
@jrcrespoh
Copy link
Contributor Author

jrcrespoh commented Aug 21, 2021 •
edited
Loading

At this point, the remaining failures are due to the quickstarts failing to pass their unit or UI tests.

Quickstarts needing test fixing:

  • AdMob
  • Authentication
  • Database (cocoapods)
  • Firestore
  • Messaging

@jrcrespoh jrcrespoh marked this pull request as ready for review August 21, 2021 06:34
@jrcrespoh
Copy link
Contributor Author

At this point, the remaining failures are due to the quickstarts failing to pass their unit or UI tests.

Quickstarts needing test fixing:

  • AdMob
  • Authentication
  • Database (cocoapods)
  • Firestore
  • Messaging

Green CI achieved by disabling testing for the above quickstarts; all other quickstarts both build and test successfully. The following spm quickstarts now run on GHA macOS 11:

  • A/B Testing
  • Crashlytics
  • Database
  • Performance

jrcrespoh
jrcrespoh commented Aug 21, 2021
View reviewed changes
scripts/test.sh
Comment on lines +83 to +85
elif [[ "$OS" == watchOS ]]; then
DESTINATION="platform=watchOS Simulator,name=${DEVICE}"
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DESTINATION is not used when targeting watchOS: previously there was trouble getting xcodebuild to use the right simulator / device, and without a -destination flag it works fine for the moment.

@paulb777 paulb777 force-pushed the jc-ci-multiplatform branch from f2f1ce9 to 93e807f Compare August 21, 2021 17:36
paulb777
paulb777 reviewed Aug 23, 2021
View reviewed changes
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Please make sure UITests are still running. For example, I don't see them in https://github.com/firebase/quickstart-ios/runs/3390100787?check_suite_focus=true

performance/SwiftUIPerformanceQuickstart/PerformanceExample/README.md Outdated Show resolved Hide resolved
scripts/test.sh Show resolved Hide resolved
@jrcrespoh
Copy link
Contributor Author

Please make sure UITests are still running. For example, I don't see them in https://github.com/firebase/quickstart-ios/runs/3390100787?check_suite_focus=true

@paulb777 Specific tests were disabled on purpose since they weren't passing: #1258 (comment)

@paulb777 paulb777 force-pushed the jc-ci-multiplatform branch from 93e807f to 64e3d3e Compare August 23, 2021 15:28
@paulb777
Copy link
Member

Were the tests disabled previously as well?

@jrcrespoh
Copy link
Contributor Author

Were the tests disabled previously as well?

They weren't disabled on purpose. Secrets were not being properly propagated (see 4f5d4a9), so the previous script simply skipped testing.

@jrcrespoh jrcrespoh requested a review from paulb777 August 23, 2021 17:35
paulb777
paulb777 approved these changes Aug 23, 2021
View reviewed changes
Copy link
Member

@paulb777 paulb777 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 great simplification/reorganization. Thanks @jrcrespoh !

I'm good to merge assuming that there is no regression in which builds and tests run.

morganchen12
morganchen12 approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM, but please wait for Paul/Gran approval before merging.

granluo
granluo approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@granluo granluo left a comment

Choose a reason for hiding this comment

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

Thanks for updating the scripts. This does simplify the workflows and increase the maintainability.

@jrcrespoh jrcrespoh changed the title Consolidate quickstarts CI testing script Consolidate quickstarts CI script Aug 23, 2021
@jrcrespoh jrcrespoh changed the title Consolidate quickstarts CI script Consolidate Quickstart CI script Aug 23, 2021
@jrcrespoh jrcrespoh merged commit 3c6f6de into master Aug 23, 2021
@jrcrespoh jrcrespoh deleted the jc-ci-multiplatform branch August 23, 2021 18:18
@jrcrespoh jrcrespoh mentioned this pull request Aug 23, 2021
Closed
5 tasks
@ncooke3
Copy link
Member

ncooke3 commented Aug 23, 2021

Thanks @jrcrespoh! This is terrific! 🎉

@jrcrespoh jrcrespoh mentioned this pull request Aug 23, 2021
Open
5 tasks
This was referenced Aug 23, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@granluo granluo granluo approved these changes

@morganchen12 morganchen12 morganchen12 approved these changes

@paulb777 paulb777 paulb777 approved these changes

@ncooke3 ncooke3 Awaiting requested review from ncooke3

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jrcrespoh @paulb777 @ncooke3 @morganchen12 @granluo