search

tinco / entity_rust

Event driven CES framework for Rust with a macro DSL
MIT License
4 stars 0 forks source link

Tests in `static_any_map.rs` are not atomic, may cause spurious failure. #1

Open kennytm opened 7 years ago

kennytm commented 7 years ago

Generally, Rust runs tests in parallel.

In https://github.com/tinco/entity_rust/blob/master/tests/static_any_map.rs, the 3 tests all operate on the same global map. Even if the map is wrapped in a SharedMutex, it is not enough to ensure thread-safety, e.g. some code in static_map_initial_set may run in the middle of static_map_multi_set, causing the test to spuriously fail.

Thread 1 Thread 2
static_map_initial_set()
static_map_multi_set()
my_map::clear_all()
my_map::push(&name, i)
my_map::clear_all()
let queue = my_map::get(&name)
Received an empty queue
assert_eq!(queue.len(), 1); but queue.len() == 0
💥

The tests can be serialized using a mutex, see rust-lang/rust#43155.

tinco commented 7 years ago

Hi @kennytm! Thanks for reporting this, I actually have known about this issue and worked around it by running the test suite in single threaded mode somehow, it's been a while and I forgot the details.

How did you come across this repo? I worked very actively on this for a while, but I then started renovating my house which has taken the better of my free time the past year. In the meanwhile a similar entity system project managed to put out a very usable library so I think there's no use for this project any longer. I guess I should pull it from Cargo, did it break your automatic ecosystem testing or something? :)

kennytm commented 7 years ago

@tinco Hi,

Before we release a new version of the Rust compiler, or introduce a potentially backward-incompatible feature (this case), we will run a tool called "cargobomb" that tests the new compiler against all 11000 packages on crates.io, to ensure we don't accidentally break our stability guarantee.

However, some crates, like yours, contain "flaky tests" which caused cargobomb to report a regression, where the problem doesn't really originate from the compiler. While we do blacklist these crates, I'd like to inform their authors as well, so future versions can continue participate in the experiment.

You don't need to pull it from crates.io (we always do manual inspection since there are really a lot of false positives). Nevertheless, fixing the tests would be even better 😄.