Skip to content

Commit c8e3561

Browse files
committed
OP#10283: Say in which way reading a track file failed
The message goes straight to the user, and it claimed the file did not say which video it belonged to whichever way reading it had failed. A file that never arrived was reported as one whose metadata was lacking, which sends whoever reads it looking inside a file that is not there. That is how the recent CI failure came to be read as a format problem rather than a missing one. Missing, unopenable, unparseable and lacking video metadata now each say so. The parser's own complaint is left out of the unparseable case: ijson quotes the offending line and points a caret into it, which is a diagram nobody can read in a popup, and the traceback still carries it to the log. Also ignore the directory S3_USER_SOURCE stages downloads in, so a stray `git add -A` cannot commit a bucket's worth of tracks and videos.
1 parent 1f65bd2 commit c8e3561

3 files changed

Lines changed: 61 additions & 8 deletions

File tree

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ data/
44
tests/data/OTCamera19_FR20_2023-05-24*
55
tests/scripts/
66
.envrc
7+
# Where S3_USER_SOURCE stages downloaded tracks and videos while running
8+
.cache-otanalytics/
79

810
# Anthropic Claude
911
.claude/rules/shared

‎OTAnalytics/plugin_s3/s3_file_providers.py‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,15 +66,31 @@ def read_video_name(ottrk: Path) -> str:
6666
str: the video file name, with extension.
6767
6868
Raises:
69-
UnreadableTrackFile: if the file carries no video metadata.
69+
UnreadableTrackFile: if the file is missing, cannot be read, or carries
70+
no video metadata. Each says which of the three it was, because the
71+
message is what the user is shown and they lead to different places
72+
to look.
7073
"""
7174
try:
7275
metadata = metadata_from_json_events(parse_json_bz2_events(ottrk))
76+
except FileNotFoundError as cause:
77+
raise UnreadableTrackFile(f"'{ottrk.name}' is missing: {ottrk}.") from cause
78+
except OSError as cause:
79+
raise UnreadableTrackFile(
80+
f"'{ottrk.name}' could not be read: {cause}."
81+
) from cause
82+
except ijson.JSONError as cause:
83+
# The parser reports the offending line with a caret pointing into it,
84+
# which belongs in the log, not in a dialog the user has to read.
85+
raise UnreadableTrackFile(
86+
f"'{ottrk.name}' is not a valid track file."
87+
) from cause
88+
try:
7389
video = metadata[ottrk_dataformat.VIDEO]
7490
return str(video[ottrk_dataformat.FILENAME]) + str(
7591
video[ottrk_dataformat.FILETYPE]
7692
)
77-
except (KeyError, TypeError, OSError, ijson.JSONError) as cause:
93+
except (KeyError, TypeError) as cause:
7894
raise UnreadableTrackFile(
7995
f"'{ottrk.name}' does not say which video it belongs to."
8096
) from cause

‎tests/unit/OTAnalytics/plugin_s3/test_read_video_name.py‎

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,15 +66,50 @@ def test_a_track_file_without_video_metadata_is_reported(
6666
) -> None:
6767
given = create_given(tmp_path, {"metadata": {}})
6868

69-
with pytest.raises(UnreadableTrackFile):
69+
with pytest.raises(UnreadableTrackFile, match="does not say which video"):
7070
read_video_name(given.ottrk)
7171

72-
def test_an_unreadable_track_file_is_reported(self, tmp_path: Path) -> None:
72+
def test_a_track_file_that_is_not_valid_json_is_reported(
73+
self, tmp_path: Path
74+
) -> None:
75+
"""Naming the content as the problem, not the video metadata."""
76+
given = create_given(tmp_path, None)
77+
78+
with pytest.raises(UnreadableTrackFile, match="is not a valid track file"):
79+
read_video_name(given.ottrk)
80+
81+
def test_a_track_file_that_cannot_be_opened_is_reported(
82+
self, tmp_path: Path
83+
) -> None:
84+
given = Given(ottrk=tmp_path / "plain.ottrk")
85+
given.ottrk.write_bytes(b"not compressed at all")
86+
87+
with pytest.raises(UnreadableTrackFile, match="could not be read"):
88+
read_video_name(given.ottrk)
89+
90+
def test_every_report_stays_on_one_line(self, tmp_path: Path) -> None:
91+
"""The message is shown in a dialog, so a parser dump does not belong.
92+
93+
ijson names the offending line and points a caret into it, which is
94+
several lines of diagram nobody can read in a popup.
95+
"""
7396
given = create_given(tmp_path, None)
7497

75-
with pytest.raises(UnreadableTrackFile):
98+
with pytest.raises(UnreadableTrackFile) as reported:
7699
read_video_name(given.ottrk)
77100

78-
def test_a_missing_track_file_is_reported(self, tmp_path: Path) -> None:
79-
with pytest.raises(UnreadableTrackFile):
80-
read_video_name(tmp_path / "absent.ottrk")
101+
assert "\n" not in str(reported.value)
102+
103+
def test_a_missing_track_file_says_it_is_missing(self, tmp_path: Path) -> None:
104+
"""A file that never arrived is a different problem from a corrupt one.
105+
106+
Reporting it as missing video metadata sends whoever reads the message
107+
looking inside a file that is not there.
108+
"""
109+
absent = tmp_path / "absent.ottrk"
110+
111+
with pytest.raises(UnreadableTrackFile, match="is missing") as reported:
112+
read_video_name(absent)
113+
114+
assert str(absent) in str(reported.value)
115+
assert isinstance(reported.value.__cause__, FileNotFoundError)

0 commit comments

Comments
 (0)