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.

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

Add sine integration test #110

Merged

Conversation

simlay
Copy link
Member

@simlay simlay commented Apr 5, 2024

Follow up from #109.

examples/sine.rs Show resolved Hide resolved
@HEnquist
Copy link
Contributor

HEnquist commented Apr 7, 2024

This adds a nearly identical copy of the sine example. Wouldn't it be more elegant to just let the ci compile the existing example?

@simlay
Copy link
Member Author

simlay commented Apr 7, 2024

This adds a nearly identical copy of the sine example. Wouldn't it be more elegant to just let the ci compile the existing example?

Yeah, I'm probably gonna remove that integration test. My understanding is that cargo test already compiles the examples but doesn't run them. I'm on the fence of actually wanting cargo test to make sound. On the macOS-12 and 13 GitHub runners, it passes. On macOS-14 (this is an m1), it errors with libunwind: malformed __unwind_info at 0x1980AAAAC bad second level page. I ended up wanting something like this test because in #109, the sine example broke.

@HEnquist
Copy link
Contributor

HEnquist commented Apr 8, 2024

Can't we just let the ci run the examples? Like

cargo run --example sine
cargo run --example something_else
cargo run --example and_one_more

If one of them fails that should fail the job.

@simlay
Copy link
Member Author

simlay commented Apr 8, 2024

Can't we just let the ci run the examples? Like

cargo run --example sine
cargo run --example something_else
cargo run --example and_one_more

If one of them fails that should fail the job.

Oh there's not really a good reason. The OCD in me says "Because an example is not a test and CI is for tests". I took your advice and added the the example run to CI.

Anyway, @HEnquist I bumped the version to 0.12.0 and CI will publish this to crates.io when this merges. As there's not a lot of other viewers, give me a review/approval at your leisure.

@HEnquist
Copy link
Contributor

Looks great!
One question, not really related to these changes. All the ci jobs install clang and llvm. Is that really needed? I don't do that in another project that uses coreaudio-rs, and it builds fine without.

@simlay
Copy link
Member Author

simlay commented Apr 12, 2024

All the ci jobs install clang and llvm. Is that really needed? I don't do that in another project that uses coreaudio-rs, and it builds fine without.

It's not really. I mostly went with it to be consistent with the ci in coreaudio-sys and use an explicit version of LLVM. The existing llvm install parts use brew install llvm which changes whenever homebrew updates the version. Also, my understanding is that brew install <thing> causes a brew update resulting in slower CI.

At some point it'd be good to test multiple version of llvm to avoid things like RustAudio/coreaudio-sys#85.

@simlay simlay merged commit b671130 into RustAudio:master Apr 12, 2024
5 checks passed
@simlay simlay deleted the fix-sine-example-and-add-sine-integration-test branch April 12, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants