rerun icon indicating copy to clipboard operation
rerun copied to clipboard

FileSink does not cause python unit tests to fail on errors

Open timsaucer opened this issue 9 months ago • 3 comments

Describe the bug

If you have an error that occurs during spawn_and_stream it gets logged as an error on the terminal but it the process returns successfully. This can cause unit test to succeed when they otherwise should fail.

To Reproduce Steps to reproduce the behavior:

  1. Change the method write_arrow_to_bytes in crates/store/re_log_encoding/src/codec/arrow.rs to immediately return an error.
  2. Build normally.
  3. Run unit test test_roundtrip_send in rerun_py/tests/unit/test_dataframe.py
  4. Unit test succeeds because we do not raise any assertions from the spawned task errors only being reported to the terminal

Expected behavior

An error message in the terminal should cause unit tests to fail.

Additional context

A proof of concept solution to this is below.I have changed two lines, using capfd fixture and the final assert to make sure "ERROR" doesn't occur in stderr. There is an open question as to whether warnings should also cause unit test failures.

If implemented, we would want to this across most or all of our py unit tests.

    def test_roundtrip_send(self, capfd) -> None:
        df = self.recording.view(index="my_index", contents="/**").select().read_all()

        with tempfile.TemporaryDirectory() as tmpdir:
            rrd = tmpdir + "/tmp.rrd"

            with rr.RecordingStream("rerun_example_test_recording") as rec:
                rec.save(rrd)
                rr.dataframe.send_dataframe(df, rec=rec)

            round_trip_recording = rr.dataframe.load_recording(rrd)

        df_round_trip = round_trip_recording.view(index="my_index", contents="/**").select().read_all()

        print("df:")
        print(df)
        print()

        print("df_round_trip:")
        print(df_round_trip)
        print()

        assert df == df_round_trip

        assert "ERROR" not in capfd.readouterr().err

timsaucer avatar Mar 26 '25 11:03 timsaucer

If the error originates in Python, then PYTHONWARNINGS="error" should catch it:

  • https://github.com/rerun-io/rerun/pull/9384

If the error originates in Rust, then setting RERUN_PANIC_ON_WARN=1 should catch it.

emilk avatar Mar 26 '25 12:03 emilk

Do you think we should add these variables to py-test in pixi.toml then? Seems easy enough

timsaucer avatar Mar 26 '25 12:03 timsaucer

Yes, I think we should set these three everywhere in our CI:

PYTHONWARNINGS="error"
RERUN_PANIC_ON_WARN=1
RERUN_STRICT=1

emilk avatar Mar 26 '25 12:03 emilk