Hotfix/test audiodataset

[mathieudpnt]

added a condition in BaseDataset class and a test to raise an error if begin > end

[Gautzilla]

Shouldn't it be if begin >= end, as the error message specifies must be smaller than? (And that a 0s-long dataset wouldn't make much sense)

[mathieudpnt]

[Gautzilla]

Actually, I'm wondering if this logic shouldn't be put in the Event class:

At least for the begin > end part (I guess an Event could be instantaneous)?

[Gautzilla]

Can't we directly use the setters to avoid testing the values twice?

[Gautzilla]

Why not >=?

[Gautzilla]

Why not <=?

[Gautzilla]

We might use classic conditional raising here

[mathieudpnt]

[Gautzilla]

[Gautzilla]

The test looks a bit clumsy, with every parameter being passed independently and the Events being instantiated within the test function.
Moreover, you have both expected_values and the expectation error parameter: you can merge both in a single parameter using pytest conditional raising.

Example of a rework that looks a bit more straightforward to me:

@pytest.mark.parametrize(
    ("event", "updated_begin", "updated_end", "expected"),
    [
        pytest.param(
            Event(
                begin=Timestamp("2024-01-01 00:00:00"),
                end=Timestamp("2024-01-02 00:00:00"),
            ),
            Timestamp("2024-01-01 12:00:00"),
            None,
            nullcontext(
                Event(
                    begin=Timestamp("2024-01-01 12:00:00"),
                    end=Timestamp("2024-01-02 00:00:00"),
                )
            ),
            id="valid begin",
        ),
        pytest.param(
            Event(
                begin=Timestamp("2024-01-01 00:00:00"),
                end=Timestamp("2024-01-02 00:00:00"),
            ),
            None,
            Timestamp("2024-01-02 12:00:00"),
            nullcontext(
                Event(
                    begin=Timestamp("2024-01-01 00:00:00"),
                    end=Timestamp("2024-01-02 12:00:00"),
                )
            ),
            id="valid end",
        ),
        pytest.param(
            Event(
                begin=Timestamp("2024-01-01 00:00:00"),
                end=Timestamp("2024-01-02 00:00:00"),
            ),
            Timestamp("2024-01-03 00:00:00"),
            None,
            pytest.raises(ValueError, match="`end`.*must be greater than `begin`.*"),
            id="invalid begin > end",
        ),
        pytest.param(
            Event(
                begin=Timestamp("2024-01-01 00:00:00"),
                end=Timestamp("2024-01-02 00:00:00"),
            ),
            None,
            Timestamp("2023-12-31 23:59:59"),
            pytest.raises(ValueError, match="`end`.*must be greater than `begin`.*"),
            id="invalid end < begin",
        ),
        pytest.param(
            Event(
                begin=Timestamp("2024-01-01 00:00:00"),
                end=Timestamp("2024-01-01 01:00:00"),
            ),
            Timestamp("2024-01-01 01:00:00"),
            None,
            pytest.raises(ValueError, match="`end`.*must be greater than `begin`.*"),
            id="begin and end equals",
        ),
    ],
)
def test_event_begin_end_updates(
    event: Event,
    updated_begin: Timestamp | None,
    updated_end: Timestamp | None,
    expected: Event,
) -> None:
    def update_event(
        event: Event, begin: Timestamp | None, end: Timestamp | None
    ) -> Event:
        if begin:
            event.begin = begin
        if end:
            event.end = end
        return event

    with expected as e:
        assert update_event(event, updated_begin, updated_end) == e

[Gautzilla]

Also, you didn't write a test function that tests the validity of the event within the constructor, it might be a good idea to do so 🥸

[mathieudpnt]

[Gautzilla]

[Gautzilla]

Also, you didn't write a test function that tests the validity of the event within the constructor, it might be a good idea to do so 🥸

I'll do it if you want, it'll allow me to check on my GH logging

[Gautzilla]

I opened this PR to add some tests to the Event class