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

(fix #52)Add integration tests for package managers #121

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

(fix #52)Add integration tests for package managers #121

wants to merge 21 commits into from

Conversation

luoxiu
Copy link
Member

@luoxiu luoxiu commented Jun 1, 2021

No description provided.

@luoxiu luoxiu changed the title Add integration tests for package managers (fix #52)Add integration tests for package managers Jun 1, 2021
@luoxiu luoxiu marked this pull request as ready for review June 1, 2021 01:30
@luoxiu luoxiu requested a review from ddddxxx June 1, 2021 01:31
@ddddxxx
Copy link
Member

ddddxxx commented Jun 1, 2021

Would you please remove latest swift (failing tests) for now? so we can get clean ci result. Also add a # FIXME mark, I'll look into it later.

@luoxiu
Copy link
Member Author

luoxiu commented Jun 2, 2021

🤔
https://github.com/cx-org/CombineX/runs/2723742441?check_suite_focus=true#step:3:396

ddddxxx
ddddxxx reviewed Jun 2, 2021
View reviewed changes
Copy link
Member

@ddddxxx ddddxxx left a comment

Choose a reason for hiding this comment

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

What is the purpose of
the extra folder package-manager in IntegrationTests/? I prefer a flatter structure. Probably IntegrationTests/[SwiftPackage|Carthage|CocoaPods]/?

.github/workflows/ci.yml

integration_tests_package_manager:
strategy:
fail-fast: false
Copy link
Member

Choose a reason for hiding this comment

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

It seems the test will pass even if build failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

fail-fast means a failed job will not fail the whole matrix.

https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

Copy link
Member

Choose a reason for hiding this comment

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

I see. So what we actually need is set -o pipefail

Copy link
Member Author

Choose a reason for hiding this comment

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

Today I learned, thanks!

IntegrationTests/package-manager/test-carthage/Cartfile Outdated Show resolved Hide resolved
@luoxiu
Copy link
Member Author

luoxiu commented Jun 2, 2021 •
edited
Loading

Still here? Better use relative path or ignore it.

Will add this to gitignore, 🤦🏻‍♀️, carthage doesn't support relative path as far as I know.

@luoxiu
Copy link
Member Author

luoxiu commented Jun 2, 2021

What is the purpose of
the extra folder package-manager in IntegrationTests/? I prefer a flatter structure. Probably IntegrationTests/[SwiftPackage|Carthage|CocoaPods]/?

Will add more integration tests in the future, for example, test the integration with CxCocoa

@ddddxxx
Copy link
Member

ddddxxx commented Jun 2, 2021

for example, test the integration with CxCocoa

Does such test belong to CXCocoa?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ddddxxx ddddxxx ddddxxx left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luoxiu @ddddxxx