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

Task: add CI job to run cargo test #44

Merged
merged 2 commits into from
Jun 15, 2023
Merged

Task: add CI job to run cargo test #44

merged 2 commits into from
Jun 15, 2023

Conversation

Kelvinyu1117
Copy link

This PR is for addressing issue #8 .

@github-actions github-actions bot added the build label Jun 12, 2023
Copy link
Owner

@laysakura laysakura left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I left several comments.

.github/workflows/rust_tests.yml Outdated Show resolved Hide resolved
matrix:
os:
[
[self-hosted, ubuntu-20.04],
Copy link
Owner

Choose a reason for hiding this comment

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

Could you tell me the reason why you use self-hosted Ubuntu rather than ubuntu-latest?

Copy link
Author

@Kelvinyu1117 Kelvinyu1117 Jun 13, 2023

Choose a reason for hiding this comment

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

The reason for that is that I saw the README inside the directory .github/workflows and the unit test workflow of other languages like Java/Go/Typescript, all of them used the self-hosted Ubuntu. However, I found that it is probably better for us to use the latest as we don't really host the machines.

schedule:
- cron: "10 2 * * *"
push:
branches: ["master", "release-*", "rust"]
Copy link
Owner

Choose a reason for hiding this comment

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

We are currently using rust_sdk branch, not rust.

Suggested change
branches: ["master", "release-*", "rust"]
branches: ["master", "release-*", "rust_sdk"]

Comment on lines 76 to 81
os:
[
[self-hosted, ubuntu-20.04],
macos-latest,
[self-hosted, windows-server-2019],
]
Copy link
Owner

Choose a reason for hiding this comment

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

IMO, we don't need need to run clippy from various OSes.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, let me fix it.

Comment on lines 106 to 111
os:
[
[self-hosted, ubuntu-20.04],
macos-latest,
[self-hosted, windows-server-2019],
]
Copy link
Owner

Choose a reason for hiding this comment

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

IMO, we don't need need to run cargo fmt from various OSes.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, let me fix it.

strategy:
fail-fast: false
matrix:
os: [[ubuntu-latest], macos-latest, [windows-latest]]
Copy link
Owner

Choose a reason for hiding this comment

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

Still using 3 OSes. IMO, ubuntu-latest here would be enough.
See: #44 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

strategy:
fail-fast: false
matrix:
os: [[ubuntu-latest], macos-latest, [windows-latest]]
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto.

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

strategy:
fail-fast: false
matrix:
os: [[ubuntu-latest], macos-latest, [windows-latest]]
Copy link
Owner

Choose a reason for hiding this comment

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

[nits] We shouldn't need nested array here.

Suggested change
os: [[ubuntu-latest], macos-latest, [windows-latest]]
os: [ubuntu-latest, macos-latest, windows-latest]

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, it seems not fixed.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for that, I have removed windows-latest and added a TODO now.

toolchain: stable
- name: Run Cargo test
working-directory: ./sdks/rust
run: cargo test -- --skip target/debug
Copy link
Owner

Choose a reason for hiding this comment

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

Tests in Windows is failing, maybe because of directory separator (/, /).
https://github.com/laysakura/beam/actions/runs/5257026192/jobs/9504117799?pr=44

Copy link
Author

Choose a reason for hiding this comment

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

Probably need to have a look at Windows environment in detail.

Copy link
Owner

Choose a reason for hiding this comment

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

Or you might skip windows-latest for now (with some TODO comment).

@laysakura laysakura merged commit 87e1799 into laysakura:rust_sdk Jun 15, 2023
@laysakura
Copy link
Owner

@Kelvinyu1117 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants