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

Exclude some long-running tests when running under miri #1863

Merged
merged 5 commits into from
Jun 13, 2022

Conversation

jhorstmann
Copy link
Contributor

@jhorstmann jhorstmann commented Jun 13, 2022

Which issue does this PR close?

Closes #1862.

Rationale for this change

Stacked borrow checking uses a lot of cpu and memory resources. Disabling it should make CI much more reliable.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 13, 2022
@jhorstmann
Copy link
Contributor Author

This did not seem to fix CI. Test tests still seem to get killed and then retried by miri.sh:

test compute::kernels::cast_utils::tests::string_to_timestamp_timezone_space ... ok
error: test failed, to rerun pass '-p arrow --lib'
Caused by:
  process didn't exit successfully: `/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri /home/runner/work/arrow-rs/arrow-rs/target/miri/x86_64-unknown-linux-gnu/debug/deps/arrow-ca24f64806e3ede9 --skip csv --skip ipc --skip json` (exit status: 255)
test compute::kernels::comparison::tests::ilike_utf8_scalar_escape_testing ... Starting Arrow MIRI run...
    Finished test [unoptimized + debuginfo] target(s) in 0.38s
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/arrow-ca24f64806e3ede9)
running 1038 tests
test array::array::tests::test_array_ref_as_array ... ok

@jhorstmann jhorstmann marked this pull request as draft June 13, 2022 13:15
@jhorstmann
Copy link
Contributor Author

error: test failed, to rerun pass '-p arrow --lib'
Caused by:
  process didn't exit successfully: `/home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri /home/runner/work/arrow-rs/arrow-rs/target/miri/x86_64-unknown-linux-gnu/debug/deps/arrow-ca24f64806e3ede9 --skip csv --skip ipc --skip json` (exit status: 255)
test compute::kernels::comparison::tests::ilike_utf8_scalar_escape_testing ... Miri finished with exit code 255

That's actually not the error code I expected if it is an out of memory issue

@jhorstmann
Copy link
Contributor Author

Disabling stacked-borrow checking is not perfect, but probably better to run miri with one less check than not running it at all. The check using a lot of cpu and memory is a known problem: rust-lang/miri#1367

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

This seems sensible 👍. We could conceivably run the full MIRI suite as part of release validation, or something, but I also don't feel particularly strongly about this

@jhorstmann jhorstmann marked this pull request as ready for review June 13, 2022 18:37
@codecov-commenter
Copy link

Codecov Report

Merging #1863 (0d265c0) into master (fb697ce) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1863      +/-   ##
==========================================
- Coverage   83.49%   83.47%   -0.03%     
==========================================
  Files         201      201              
  Lines       56902    56987      +85     
==========================================
+ Hits        47511    47570      +59     
- Misses       9391     9417      +26     
Impacted Files Coverage Δ
arrow/src/array/array_union.rs 90.92% <ø> (ø)
arrow/src/compute/kernels/filter.rs 88.33% <ø> (ø)
arrow/src/util/bit_chunk_iterator.rs 93.83% <ø> (ø)
parquet_derive/src/parquet_field.rs 65.75% <0.00%> (-0.23%) ⬇️
parquet/src/encodings/encoding.rs 93.46% <0.00%> (-0.20%) ⬇️
parquet/src/arrow/async_reader.rs 0.00% <0.00%> (ø)
arrow/src/datatypes/datatype.rs 65.79% <0.00%> (+0.37%) ⬆️
parquet/src/file/serialized_reader.rs 94.95% <0.00%> (+0.49%) ⬆️
arrow/src/array/array_binary.rs 94.24% <0.00%> (+0.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb697ce...0d265c0. Read the comment docs.

Copy link
Contributor

@tustvold tustvold left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude some long-running tests from miri
3 participants