-
Notifications
You must be signed in to change notification settings - Fork 256
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
feat: add compiled code size tests #412
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/// Compiles contract to wasm with release configuration and returns the code size. | ||
fn check_example_size(example: &str) -> usize { | ||
let status = std::process::Command::new("cargo") | ||
Comment on lines
+2
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, that seems like a bad implication to update a user's toolchain unknowingly through running a test, maybe if this test was ignored by default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems so, but I don't think that's the case. Like, when you run Really, Empirically, I've been using rustup commands in CI in various projects (example from rust-analyzer: https://github.com/rust-analyzer/rust-analyzer/blob/master/xtask/src/main.rs#L80-L89), and it never caused problems. But maybe I am biased -- I am pretty found of various toolchain hacks which streamline workflows :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, that's a fair point. I'll make this change |
||
.env("RUSTFLAGS", "-C link-arg=-s") | ||
.args(&["build", "--release", "--target", "wasm32-unknown-unknown", "--manifest-path"]) | ||
.arg(format!("../examples/{}/Cargo.toml", example)) | ||
.status() | ||
.unwrap(); | ||
if !status.success() { | ||
panic!("building wasm example returned non-zero code {}", status); | ||
} | ||
|
||
let wasm = std::fs::read(format!( | ||
"../examples/{}/target/wasm32-unknown-unknown/release/{}.wasm", | ||
example, | ||
example.replace("-", "_") | ||
)) | ||
.unwrap(); | ||
|
||
wasm.len() | ||
} | ||
|
||
#[test] | ||
fn lock_fungible_code_size_check() { | ||
let size = check_example_size("lockable-fungible-token"); | ||
|
||
// Current contract size at the time of writing this test is 164_433, giving about ~10% buffer. | ||
assert!(size < 180_000); | ||
} | ||
|
||
#[test] | ||
fn status_message_code_size_check() { | ||
let size = check_example_size("status-message"); | ||
|
||
// Currently 140823. | ||
assert!(size < 155_000); | ||
} |
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.
If I am not mistaken, this should not be necessary?
actions-rs/toolchain@v1
already installs this target, no?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.
yeah that's what I assumed, but there might be something internal which installs it for a different toolchain, not sure. Just added that as a workaround and was going to look into it today
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.
obesrvation: this yml sets a horribly outdated toolchain at the start
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.
yeah this CI is something I would like to improve/rewrite in the future, as it only checks formatting and runs tests, to give some context into why I thought just to add a workaround for now. I can create an issue for this now, because I think it's a standard to at least check clippy as well.