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.

Sign up for GitHub

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

Jump to bottom

Github Actions #26

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Github Actions #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c17d313
Github actions to run tests & formatter
amika-sq Dec 1, 2023
07e5c0b
cargo fmt --all
amika-sq Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
name: SDK Rust CI

on:
push:
branches:
- main
pull_request:
branches:
- '*'

env:
CARGO_TERM_COLOR: always

jobs:
test:
name: cargo test
strategy:
matrix:
os: [ ubuntu-latest, macos-latest, windows-latest ]
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-12-01 at 4 52 57 PM

Currently only take a few seconds longer than macos, so figured it would be fine to have. I'm fine removing though if people don't think it's worth it.

Copy link
Member

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...

rust: [stable, nightly]
Copy link
Contributor

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

Copy link
Contributor Author

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.

runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
- name: Set up Rust
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
toolchain: ${{ matrix.rust }}
- name: Test
run: cargo test --workspace
Comment on lines +28 to +29
Copy link
Member

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?)

Copy link
Contributor Author

@amika-sq amika-sq Dec 4, 2023 •
edited
Loading

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 run cargo 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!

format:
name: cargo fmt
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup Rustfmt
uses: actions-rust-lang/setup-rust-toolchain@v1
with:
toolchain: stable
components: rustfmt
- name: Rustfmt Check
id: rustfmt-check
uses: actions-rust-lang/rustfmt@v1
7 changes: 4 additions & 3 deletions crates/credentials/src/pex/v2/presentation_definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,11 @@ pub enum Optionality {
#[cfg(test)]
mod tests {
use super::*;
use serde_canonical_json::CanonicalFormatter;
use serde_json::Serializer;
use serde_json::{json, Value};
use std::fs;
use std::path::Path;
use serde_json::Serializer;
use serde_canonical_json::CanonicalFormatter;

#[test]
fn can_serialize() {
Expand Down Expand Up @@ -191,7 +191,8 @@ mod tests {
let json_value = String::from_utf8(ser.into_inner()).unwrap();

let mut ser = Serializer::with_formatter(Vec::new(), CanonicalFormatter::new());
let deserialized_with_type: PresentationDefinition = serde_json::from_str(&raw_string).unwrap();
let deserialized_with_type: PresentationDefinition =
serde_json::from_str(&raw_string).unwrap();
Comment on lines +194 to +195
Copy link
Member

Choose a reason for hiding this comment

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

rustfmt for the win! 💪

deserialized_with_type.serialize(&mut ser).unwrap();
let json_serialized_from_type = String::from_utf8(ser.into_inner()).unwrap();

Expand Down