-
Notifications
You must be signed in to change notification settings - Fork 6
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
Improve CI workflows #28
Conversation
this is done so we can approve/reject PRs from forks
Added installing of protoc 29.1 to fix issue in #27 |
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.
LGTM.
Sidenote while we're looking at this file:
Is this correct?:
- name: Run tests
id: run-tests
if: ${{ steps.check-tests.outcome == 'success' }}
run: |
cargo +${{ steps.install-rust.outputs.name }} test --target ${{ matrix.target }} --workspace --test "*" --no-fail-fast
It seems like providing --test "*"
means that it only runs tests found in **/tests/*.rs
but doesn't test anything in src/
folders. See the difference in output of cargo test --test "*"
and cargo test
in repo root folder.
Also if you give it a bogus name it lists only the four tests in tests/*.rs
files:
$ cargo test --test "foobar"
error: no test target named `foobar`.
Available test targets:
mpsc
zst
json_roundtrip
function_block
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.
LGTM
Good catch! It is indeed a little bit wrong! |
In this PR I changed event type due to number of reasons:
And I also bumped ubuntu runner version to 24.04 (latest) to get rid of a warning in logs.