-
Notifications
You must be signed in to change notification settings - Fork 7
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
Task: add CI job to run cargo test #44
Conversation
There was a problem hiding this 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
matrix: | ||
os: | ||
[ | ||
[self-hosted, ubuntu-20.04], |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
.github/workflows/rust_tests.yml
Outdated
schedule: | ||
- cron: "10 2 * * *" | ||
push: | ||
branches: ["master", "release-*", "rust"] |
There was a problem hiding this comment.
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
.
branches: ["master", "release-*", "rust"] | |
branches: ["master", "release-*", "rust_sdk"] |
.github/workflows/rust_tests.yml
Outdated
os: | ||
[ | ||
[self-hosted, ubuntu-20.04], | ||
macos-latest, | ||
[self-hosted, windows-server-2019], | ||
] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.github/workflows/rust_tests.yml
Outdated
os: | ||
[ | ||
[self-hosted, ubuntu-20.04], | ||
macos-latest, | ||
[self-hosted, windows-server-2019], | ||
] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
.github/workflows/rust_tests.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [[ubuntu-latest], macos-latest, [windows-latest]] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
.github/workflows/rust_tests.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [[ubuntu-latest], macos-latest, [windows-latest]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
.github/workflows/rust_tests.yml
Outdated
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [[ubuntu-latest], macos-latest, [windows-latest]] |
There was a problem hiding this comment.
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.
os: [[ubuntu-latest], macos-latest, [windows-latest]] | |
os: [ubuntu-latest, macos-latest, windows-latest] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
@Kelvinyu1117 Thanks! |
This PR is for addressing issue #8 .