Skip to content
This repository was archived by the owner on May 10, 2025. It is now read-only.

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

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

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

luoxiu 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
ddddxxx reviewed Jun 1, 2021
View reviewed changes
@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

@ddddxxx ddddxxx Jun 2, 2021

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

@luoxiu luoxiu Jun 2, 2021

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

@ddddxxx ddddxxx Jun 2, 2021

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

@luoxiu luoxiu Jun 3, 2021

Choose a reason for hiding this comment

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

Today I learned, thanks!

@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ddddxxx ddddxxx ddddxxx left review comments

Reviewers whose approvals may not affect merge requirements

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