Stop ignoring tests#7844
Conversation
Signed-off-by: Robert Kruszewski <github@robertk.io>
|
|
||
| #[test] | ||
| #[ignore = "apply() has a bug with null propagation from struct validity to non-nullable child fields"] | ||
| #[should_panic = "Cannot create null scalar with non-nullable dtype i32"] |
There was a problem hiding this comment.
This doesn't seem right? Is this actually the correct behavior?
There was a problem hiding this comment.
it's not the correct behaviour but instead of ignoring the test we let it run and assert it panics. This should be fixed and if someone fixes it this test will start failing
There was a problem hiding this comment.
Sure, but now nobody is tracking this issue? And now we are overloading should_panic with both "this is a test that should panic" and "this is a bug that someone needs to fix, without an actual reason". That was why I had those ignores, because they are easy to find.
Merging this PR will improve performance by 19.63%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | patched_take_10k_adversarial |
261.2 µs | 230.9 µs | +13.12% |
| ⚡ | Simulation | patched_take_10k_dispersed |
318.4 µs | 288.1 µs | +10.52% |
| ⚡ | Simulation | patched_take_10k_first_chunk_only |
304.7 µs | 274.4 µs | +11.06% |
| ⚡ | Simulation | take_10k_first_chunk_only |
273.1 µs | 228.3 µs | +19.63% |
| ⚡ | Simulation | take_10k_dispersed |
286.7 µs | 241.8 µs | +18.55% |
Comparing rk/ignores (9fffef6) with develop (b212667)
These were ignored for reasons unknown. I removed ignores, fixed one case of
actually failing test and converted one of them to an expected panic
fix #7835