-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
ci: add ignore-wasm
to failing wasm tests
#7537
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
base: master
Are you sure you want to change the base?
Conversation
Please also revert #7518 . |
These annotations are only for a small section of the failing wasm tests, which yields: I plan on opening PRs for the remaining sections to it split up based on module, unless it is preferred to aggregate all annotations here |
Thank you for the PR!
I think we would want to have 1 PR (1 commit), instead of having multiple commits of this. Having some large file changes for this is fine with me. |
ignore-wasm
to failing wasm testsignore-wasm
to failing wasm tests
Test results yield: |
Now the doc test is hitting Lines 463 to 475 in 925c614
(it looks like You could update the cfgs in the ci settings like this: - cargo test -p tokio --target ${{ matrix.target }} --features full
+ cargo test -p tokio --target ${{ matrix.target }} --features "sync,macros,io-util,rt,time" |
8fb85d5
to
ab2a5f5
Compare
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.
Looks good to me.
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.
See #7537 (comment), It might be a cargo issue, we'd better having a minimal reproducible example, otherwise, it is hard for me to say that I know what I am doing.
I made an minimal reproducible example, it seems unrelated to neither Rust version nor wasm. If I understand correctly, since there is a @mox692 Do you have any idea? It look like a Cargo issue. [package]
name = "feature-flag"
version = "0.1.0"
edition = "2024"
[dependencies]
[features]
epoll = []
[lints.rust]
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(my_unstable)'] } // src/lib.rs
#[cfg(all(
not(my_unstable),
any(
feature = "epoll",
)
))]
compile_error!("blablabla"); # no error
RUSTFLAGS="--cfg my_unstable" cargo +1.88 test --lib --features epoll
RUSTFLAGS="--cfg my_unstable" cargo +1.89 test --lib --features epoll
# compilation error
RUSTFLAGS="--cfg my_unstable" cargo +1.88 test --doc --features epoll
RUSTFLAGS="--cfg my_unstable" cargo +1.89 test --doc --features epoll |
Not sure. It looks like cfg isn't passed to cargo properly. It's probably worth filing an issue on cargo side? |
@ADD-SP This does not trigger # no error on --doc
RUSTFLAGS="--cfg my_unstable" RUSTDOCFLAGS="--cfg my_unstable" cargo +1.88 test --doc --features epoll
RUSTFLAGS="--cfg my_unstable" RUSTDOCFLAGS="--cfg my_unstable" cargo +1.89 test --doc --features epoll
# no error on --lib
RUSTFLAGS="--cfg my_unstable" cargo +1.88 test --lib --features epoll
RUSTFLAGS="--cfg my_unstable" cargo +1.89 test --lib --features epoll If we run RUSTDOCFLAGS="--cfg my_unstable" cargo +1.88 test --doc --features epoll
Compiling foo v0.1.0 (/.../foo)
error: blablabla
--> src/lib.rs:8:1
|
8 | compile_error!("blablabla");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: could not compile `foo` (lib) due to 1 previous error RUSTFLAGS="--cfg my_unstable" cargo +1.88 test --doc --features epoll
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.19s
Doc-tests foo
error: blablabla
--> src/lib.rs:8:1
|
8 | compile_error!("blablabla");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to 1 previous error
error: doctest failed, to rerun pass `--doc` I believe this boils down to how running |
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 think there are several tests don't need to be ignore-wasm
, we could enable it.
By the way, and not a blocker, I think its ok to remove the async fn main
in the rendered docs.
/// # /*
/// #[tokio::main]
/// # */
/// # #[tokio::main(flavor = "current_thread")]
/// async fn main() {
/// let mut none = stream::empty::<i32>();
///
/// assert_eq!(None, none.next().await);
/// }
///
So we can do this, and it looks less confusing while reading the tokio source.
/// # #[tokio::main(flavor = "current_thread")]
/// async fn main() {
/// let mut none = stream::empty::<i32>();
///
/// assert_eq!(None, none.next().await);
/// # }
///
tokio-util/src/io/sync_bridge.rs
Outdated
@@ -77,7 +77,7 @@ use tokio::io::{ | |||
/// incrementally. This avoids blocking and improves performance over using | |||
/// `SyncIoBridge`. | |||
/// | |||
/// ```rust | |||
/// ```rust,ignore-wasm |
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.
Could we enable this test?
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 initially couldn't get this test to be enabled until I lowered the memory allocated in the test from 64KiB to 16KiB, specifically for the threads runner.
I tried using futures::executor::block_on
and adjusting the max memory limit of the test in RUSTFLAGS
but neither seemed to work. Neither did simply adjusting the flavor to current_thread
.
Not sure what the deeper problem is there. Except maybe it's just due to the single-threaded execution?
I removed tokio/tokio-stream/src/stream_ext.rs Lines 515 to 518 in 58f199e
tokio/tokio-util/src/io/sync_bridge.rs Lines 107 to 110 in 58f199e
|
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 think there are still some doc tests should not be disabled on wasm, this is still not a exhausted list, you might find more by scanning this PR.
I use this screenshot to illustrate my concerns.

I rendered the docs of tokio::task
using this PR. Using too many exclamation marks in docs can make it a less enjoyable read for downstream.
/// # /* | ||
/// #[tokio::main] | ||
/// # */ |
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.
left-over code?
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.
No, I left a #7537 (comment). If you look at this particular test, I don't think it's as clear cut as rendering out async fn main
as you pointed out here #7537 (comment).
I'm open to reverting this, I've just found there's similar approaches in existing doctests even before this PR such as this written by Darksonn which I believe I mistakenly changed
https://github.com/tokio-rs/tokio/blob/master/tokio-stream/src/stream_ext.rs#L340-L344
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.
JoinMap tests should not be disabled in wasm.
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.
Could we enable doc tests?
@@ -11,7 +11,7 @@ use std::task::{ready, Context, Poll}; | |||
/// | |||
/// # Example | |||
/// | |||
/// ``` | |||
/// ```ignore-wasm |
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.
Could we enable this test?
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.
Could we enable doc tests?
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.
Could we enable doc tests?
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.
Could we enable doc tests?
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.
Could we enable doc tests?
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.
Could we enable doc tests?
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.
Could we enable doc tests?
Addresses failing wasm tests: #7519
Motivation
Currently, the MSRV is pinned temporarily and there is a large amount of failures for wasm doctests with the Rust 1.89.0 release.
Solution
Add
ignore-wasm
to failing doctests until these tests pass with the new release.