-
Notifications
You must be signed in to change notification settings - Fork 16
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
Github Actions #26
Github Actions #26
Conversation
460328e
to
c17d313
Compare
name: cargo test | ||
strategy: | ||
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.
We haven't really targeted windows in other repos. It also turns out to be pretty slow. Do you think it's worth adding?
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.
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 running Windows for Bundler Bonanza Electron apps: https://github.com/TBD54566975/bundler-bonanza/blob/main/.github/workflows/tests-runner.yml#L241
Rationale: anticipating build issues for windows developers...
strategy: | ||
matrix: | ||
os: [ ubuntu-latest, macos-latest, windows-latest ] | ||
rust: [stable, nightly] |
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.
Curious why you chose the nightly build? What do you think about removing? It would cut the testing by half :P
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.
Every six weeks, nightly is brought into a beta. Then six weeks after the first beta is released, a new stable release is made: https://doc.rust-lang.org/book/appendix-07-nightly-rust.html
I'm not opposed to removing, but I have heard that open source developers may be more inclined to use nightly for various reasons. Happy to remove though if we wanted to save resources.
let deserialized_with_type: PresentationDefinition = | ||
serde_json::from_str(&raw_string).unwrap(); |
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.
rustfmt for the win! 💪
- name: Test | ||
run: cargo test --workspace |
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.
Do you think it's worth adding a cargo build
to make sure our builds are passing as well? (Or is it embedded in the cargo test
command already?)
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.
I went ahead and did a little digging here to find out the specifics:
cargo test
will implicitly runcargo build
, BUT it will also build code that is specifically marked with#[cfg(test)]
or#[test]
(in addition to actually running the tests themselves).
Our upcoming bindings code will rely on artifacts generated via cargo build
, as we don't want to include test code as a part of our bindings. I'll go ahead and add this in as part of the bindings CI once we get there!
Add Github actions to do the following:
cargo test --workspace
, to run all the tests within this Rust workspace.cargo fmt --all
and report all formatting differences in a nice overview on the Action Summary page.There were formatting issues in the repo, so I went ahead and ran the formatter and committed the fixes!