diff --git a/tests/unit/core/runners/test_resume_gate.py b/tests/unit/core/runners/test_resume_gate.py index 76e558d4..dc31667a 100644 --- a/tests/unit/core/runners/test_resume_gate.py +++ b/tests/unit/core/runners/test_resume_gate.py @@ -8,6 +8,7 @@ from sieval.core.runners.resume_gate import ( ResumeAction, ResumeVersionError, + format_identity_reject_message, format_reject_message, resume_version_verdict, ) @@ -127,3 +128,254 @@ def test_meta_without_version_key_raises(self, tmp_path): (tmp_path / "meta.json").write_bytes(orjson.dumps({"deterministic": True})) with pytest.raises(ResumeVersionError): gate_resume_version(tmp_path, "0.6.0") + + +class TestRejectReasonIdentifiesTheRule: + """Each rejection names *which* rule fired, not merely that one did. + + The four reject reasons lead an operator to four different fixes — fix the + version string, reinstall a released build, pin a non-dev build, or match the + series. Asserting only that `reason` is non-empty lets any of them be swapped + for another without a test noticing, including a swap that sends the operator + somewhere useless. + """ + + def test_unparseable_says_so(self): + assert ( + resume_version_verdict("not-a-version", "0.6.0").reason + == "version string is unparseable" + ) + + def test_unknown_version_says_so(self): + assert ( + resume_version_verdict("0.0.0", "0.6.0").reason + == "version is unknown (0.0.0)" + ) + + def test_dev_build_says_so(self): + assert ( + resume_version_verdict("0.6.0", "0.6.1.dev3+gxyz").reason + == "development/local build cannot be matched non-exactly" + ) + + def test_local_build_shares_the_dev_reason(self): + assert ( + resume_version_verdict("0.6.0", "0.6.1+local").reason + == "development/local build cannot be matched non-exactly" + ) + + def test_series_break_says_so(self): + assert ( + resume_version_verdict("0.6.0", "0.7.0").reason + == "incompatible version series" + ) + + def test_a_permitted_resume_carries_no_reason(self): + # `reason` is documented as populated only for REJECT; a stray reason on + # an accepted resume would surface as an operator-facing message about a + # run that was never blocked. + assert resume_version_verdict("0.6.0", "0.6.0").reason == "" + assert resume_version_verdict("0.6.0", "0.6.3").reason == "" + + +class TestLadderPrecedence: + """The ladder's *order* is the contract, not just its membership.""" + + def test_exact_match_outranks_every_reject_rule(self): + # Resuming your own build is always allowed, even when the string would + # fail every later rule. + for v in ("0.0.0", "not-a-version", "0.5.1.dev24+gabc", "0.6.0+local"): + assert resume_version_verdict(v, v).action is ResumeAction.EXACT + + def test_unparseable_outranks_the_unknown_check(self): + # An unparseable string cannot be compared to 0.0.0 at all, so it must be + # reported as unparseable rather than as "unknown". + assert ( + resume_version_verdict("not-a-version", "0.0.0").reason + == "version string is unparseable" + ) + + def test_unknown_outranks_the_dev_check(self): + assert ( + resume_version_verdict("0.0.0", "0.6.1.dev3+gxyz").reason + == "version is unknown (0.0.0)" + ) + + def test_dev_outranks_the_series_check(self): + # Different series *and* a dev build: the dev build is the actionable + # one, since matching the series would still not make it resumable. + assert ( + resume_version_verdict("0.6.0", "1.2.0.dev1").reason + == "development/local build cannot be matched non-exactly" + ) + + +class TestBreakAxis: + """Under 1.0 the break axis is minor; from 1.0 on it is major.""" + + def test_patch_bumps_stay_compatible_under_1_0(self): + assert ( + resume_version_verdict("0.6.0", "0.6.99").action is ResumeAction.COMPATIBLE + ) + + def test_minor_is_the_break_axis_under_1_0(self): + assert resume_version_verdict("0.6.9", "0.7.0").action is ResumeAction.REJECT + + def test_minor_bumps_stay_compatible_from_1_0(self): + assert ( + resume_version_verdict("1.0.0", "1.99.0").action is ResumeAction.COMPATIBLE + ) + + def test_major_is_the_break_axis_from_1_0(self): + assert resume_version_verdict("1.99.0", "2.0.0").action is ResumeAction.REJECT + + def test_crossing_into_1_0_breaks(self): + # The axis itself changes here, so the pair is incompatible in both + # directions regardless of how close the numbers look. + assert resume_version_verdict("0.9.9", "1.0.0").action is ResumeAction.REJECT + assert resume_version_verdict("1.0.0", "0.9.9").action is ResumeAction.REJECT + + def test_pinnable_prereleases_are_not_treated_as_dev_builds(self): + # A pre-release is a fixed artifact, unlike dev/local; it falls through + # to the series check rather than being rejected as unpinnable. + assert ( + resume_version_verdict("0.6.0", "0.6.1rc1").action + is ResumeAction.COMPATIBLE + ) + assert ( + resume_version_verdict("0.6.0", "0.7.0rc1").reason + == "incompatible version series" + ) + + +class TestFormatIdentityRejectMessage: + """Previously untested entirely — 18 mutants with no test to see them. + + This is the message shown when a result directory was produced by a + *different* task. Its own text states the stake: a finished run is matched by + path alone, so resuming here would hand back the persisted task's report as + this task's result without running a sample. The operator can only act on it + if it names both tasks and both ways out. + """ + + def _msg(self) -> str: + return format_identity_reject_message("gsm8k_0shot_gen", "math_500_0shot_gen") + + def test_names_both_tasks(self): + msg = self._msg() + assert "gsm8k_0shot_gen" in msg + assert "math_500_0shot_gen" in msg + + def test_distinguishes_persisted_from_current(self): + # Both names appear; without labels the operator cannot tell which is + # which, and the two fixes are not symmetric. + msg = self._msg() + persisted = msg.index("gsm8k_0shot_gen") + current = msg.index("math_500_0shot_gen") + assert msg.index("persisted") < persisted < current + assert "meta.json" in msg, "the operator has to know where to look" + + def test_states_the_consequence(self): + msg = self._msg() + assert "without running a single sample" in msg + + def test_offers_both_recovery_paths(self): + msg = self._msg() + assert "Remove the result_dir and start fresh" in msg + assert "Give this task its own result_dir" in msg + + def test_leads_with_the_abort(self): + assert self._msg().startswith("Resume aborted:") + + +class TestFormatRejectMessageStructure: + """The message body, not just that four substrings appear somewhere.""" + + def _msg(self) -> str: + return format_reject_message("0.6.0", "0.7.0", "incompatible version series") + + def test_leads_with_the_abort_and_names_the_subject(self): + msg = self._msg() + assert msg.startswith("Resume aborted:") + assert "sieval version is incompatible" in msg + + def test_labels_which_version_is_which(self): + msg = self._msg() + assert msg.index("persisted") < msg.index("0.6.0") + assert msg.index("current") < msg.index("0.7.0") + assert "meta.json" in msg + + def test_carries_the_reason_under_its_label(self): + assert "reason: incompatible version series" in self._msg() + + def test_offers_both_recovery_paths(self): + msg = self._msg() + assert "Remove the result_dir and start fresh" in msg + assert "Reinstall sieval matching the persisted version series" in msg + + def test_the_two_builders_do_not_share_a_recovery_path(self): + # They abort for different reasons and the second option differs: a + # version mismatch is fixed by reinstalling, an identity mismatch by + # giving the task its own directory. Swapping them would send the + # operator down a route that cannot work. + version_msg = self._msg() + identity_msg = format_identity_reject_message("a_task", "b_task") + assert "Reinstall sieval" in version_msg + assert "Reinstall sieval" not in identity_msg + assert "its own result_dir" in identity_msg + assert "its own result_dir" not in version_msg + + +class TestUnpinnableBuildsRejectOnEitherSide: + """`local` and `dev` are independent markers; either alone must reject. + + The earlier run-side test used "0.6.1.dev3+gxyz", which carries *both*, so it + could not tell an `or` in this guard from an `and` — a build tagged only + local would have fallen through to the series check and resumed. + """ + + def test_local_only_on_the_run_side(self): + assert ( + resume_version_verdict("0.6.1+local", "0.6.0").action is ResumeAction.REJECT + ) + + def test_dev_only_on_the_run_side(self): + assert ( + resume_version_verdict("0.6.1.dev3", "0.6.0").action is ResumeAction.REJECT + ) + + def test_local_only_on_the_current_side(self): + assert ( + resume_version_verdict("0.6.0", "0.6.1+local").action is ResumeAction.REJECT + ) + + def test_dev_only_on_the_current_side(self): + assert ( + resume_version_verdict("0.6.0", "0.6.1.dev3").action is ResumeAction.REJECT + ) + + +class TestRejectMessagesExplainThemselves: + """The explanation is the load-bearing half of an operator message. + + Both builders spell out *why* the abort happened. Without it the operator + sees two versions or two task names and a pair of options, with nothing to + choose between them. + """ + + def test_identity_message_explains_the_path_match(self): + msg = format_identity_reject_message("a_task", "b_task") + assert "a finished run is matched by path alone" in msg + assert "would hand back the persisted task's report" in msg + + def test_version_message_names_the_incompatibility(self): + msg = format_reject_message("0.6.0", "0.7.0", "incompatible version series") + assert "sieval version is incompatible with the persisted run" in msg + + def test_both_messages_end_on_an_actionable_choice(self): + for msg in ( + format_identity_reject_message("a_task", "b_task"), + format_reject_message("0.6.0", "0.7.0", "series"), + ): + assert "Either:" in msg + assert " 1. " in msg and " 2. " in msg diff --git a/tests/unit/core/tasks/test_anomaly.py b/tests/unit/core/tasks/test_anomaly.py index f25bc3c8..9499ea84 100644 --- a/tests/unit/core/tasks/test_anomaly.py +++ b/tests/unit/core/tasks/test_anomaly.py @@ -5,13 +5,17 @@ """ from dataclasses import replace +from pathlib import Path +from typing import cast from unittest.mock import AsyncMock +import orjson import pytest from sieval.core.models.model import ModelOutput from sieval.core.tasks.anomaly import ( _DETECTION_RULES, + AnomalyReport, TaskAnomalyDetector, _rule_applies, _unwrap_result, @@ -445,6 +449,398 @@ async def test_save_backup_when_rules_changed(self, tmp_path, sample_model_meta) backups = list(tmp_path.glob("anomalies.*.json")) assert len(backups) == 1 + async def _save_two( + self, tmp_path, *, old_hash, new_hash="newhash", generated_at=... + ) -> TaskAnomalyDetector: + """Persist a report under *old_hash*, then save another under *new_hash*.""" + detector = TaskAnomalyDetector(root_dir=tmp_path) + ctx = _make_final_ctx(postprocess_result="answer") + + first = detector.generate_report({0: ctx}, "t", task_tags={"gen"}) + # Reached through a plain-dict view: these keys are required on the + # TypedDict, and the point of the test is a report that lacks them. + meta = cast(dict, first["meta"]) + if old_hash is None: + meta.pop("rules_hash", None) + else: + meta["rules_hash"] = old_hash + if generated_at is not ...: + if generated_at is None: + meta.pop("generated_at", None) + else: + meta["generated_at"] = generated_at + await detector.save(first, backup_if_changed=False) + + second = detector.generate_report({0: ctx}, "t", task_tags={"gen"}) + second["meta"]["rules_hash"] = new_hash + await detector.save(second, backup_if_changed=True) + return detector + + @pytest.mark.anyio + async def test_no_backup_when_the_rules_are_unchanged(self, tmp_path): + # A backup per save would fill the run directory with copies of an + # identical report, and bury the one that marks a real rule rotation. + await self._save_two(tmp_path, old_hash="samehash", new_hash="samehash") + assert list(tmp_path.glob("anomalies.*.json")) == [] + + @pytest.mark.anyio + async def test_no_backup_when_the_old_report_has_no_hash(self, tmp_path): + # Nothing to compare against, so "changed" is unknowable; overwriting is + # the documented behaviour rather than guessing a rotation happened. + await self._save_two(tmp_path, old_hash=None) + assert list(tmp_path.glob("anomalies.*.json")) == [] + + @pytest.mark.anyio + async def test_no_backup_without_a_timestamp_to_name_it(self, tmp_path): + # The backup name is derived from the old report's generated_at; with no + # timestamp there is no non-colliding name to write. + await self._save_two(tmp_path, old_hash="oldhash", generated_at=None) + assert list(tmp_path.glob("anomalies.*.json")) == [] + + @pytest.mark.anyio + async def test_the_backup_is_named_from_the_old_reports_timestamp(self, tmp_path): + # Not "now": the name has to identify *which* report was displaced, so a + # later reader can line it up with the run that produced it. + await self._save_two( + tmp_path, old_hash="oldhash", generated_at="2026-08-07T01:02:03" + ) + assert (tmp_path / "anomalies.20260807010203.json").exists() + + @pytest.mark.anyio + async def test_the_current_report_survives_the_backup(self, tmp_path): + # The old file is moved aside, not deleted, and the new one takes its + # place under the canonical name. + await self._save_two( + tmp_path, old_hash="oldhash", generated_at="2026-08-07T01:02:03" + ) + current = orjson.loads((tmp_path / "anomalies.json").read_bytes()) + assert current["meta"]["rules_hash"] == "newhash" + backup = orjson.loads((tmp_path / "anomalies.20260807010203.json").read_bytes()) + assert backup["meta"]["rules_hash"] == "oldhash" + + @pytest.mark.anyio + async def test_an_unreadable_old_report_does_not_fail_the_run(self, tmp_path): + # Backup is best-effort housekeeping; a corrupt predecessor must not + # take down the run that is trying to record its own results. + (tmp_path / "anomalies.json").write_bytes(b"not json{") + detector = TaskAnomalyDetector(root_dir=tmp_path) + ctx = _make_final_ctx(postprocess_result="answer") + report = detector.generate_report({0: ctx}, "t", task_tags={"gen"}) + await detector.save(report, backup_if_changed=True) # no raise + assert (tmp_path / "anomalies.json").exists() + + +class TestDetectionRuleRegistration: + """What `@sieval_detection_rule` derives, not just that it registers. + + The built-in rules are registered at import, so a test that only reads the + resulting registry cannot see the decorator's own logic. These exercise it + directly. The derived name is what `applies_to` and every stored report key + on, so a change here renames rules across the fleet. + """ + + def _register(self, func_name: str, **kwargs): + def rule(ctx: TaskContext) -> set[int]: + # Never fires: these tests are about registration, not detection. + # The parameter is named and typed to satisfy the DetectFunc + # protocol, which declares it positionally *and* by keyword. + del ctx + return set() + + rule.__name__ = func_name + sieval_detection_rule( + description=kwargs.pop("description", "d"), + category=kwargs.pop("category", "output_quality"), + rationale=kwargs.pop("rationale", "r"), + **kwargs, + )(rule) + return _DETECTION_RULES + + def test_the_detect_prefix_is_stripped_from_the_rule_name(self): + assert "empty_thing" in self._register("detect_empty_thing") + + def test_a_private_detect_prefix_is_stripped_too(self): + assert "empty_thing" in self._register("_detect_empty_thing") + + def test_a_name_without_the_prefix_is_kept_whole(self): + # Stripping a suffix instead, or matching case-insensitively, would + # silently rename rules that do not follow the convention. + assert "custom_rule" in self._register("custom_rule") + + def test_default_tags_come_from_the_rule_name(self): + rules = self._register("detect_empty_thing") + assert rules["empty_thing"]["definition"]["tags"] == ["empty thing"] + + def test_explicit_tags_win_over_the_derived_default(self): + rules = self._register("detect_empty_thing", tags=["explicit"]) + assert rules["empty_thing"]["definition"]["tags"] == ["explicit"] + + def test_an_empty_tag_list_is_respected_not_replaced(self): + # `is not None`, not truthiness: an explicitly empty list means "no + # tags", which is different from "derive some for me". + rules = self._register("detect_empty_thing", tags=[]) + assert rules["empty_thing"]["definition"]["tags"] == [] + + def test_severity_defaults_to_warning(self): + rules = self._register("detect_empty_thing") + assert rules["empty_thing"]["definition"]["severity"] == "warning" + + def test_severity_can_be_raised(self): + rules = self._register("detect_empty_thing", severity="error") + assert rules["empty_thing"]["definition"]["severity"] == "error" + + def test_the_definition_carries_every_declared_field(self): + # These keys are serialized into `rules_schema` and hashed; a renamed + # key changes the hash and breaks any consumer reading the report. + rules = self._register( + "detect_empty_thing", + description="a description", + category="correctness", + rationale="a rationale", + applies_to=["gen"], + threshold=3, + ) + definition = rules["empty_thing"]["definition"] + assert definition["description"] == "a description" + assert definition["category"] == "correctness" + assert definition["rationale"] == "a rationale" + assert definition["applies_to"] == ["gen"] + assert definition["threshold"] == 3 + + def test_the_registered_function_is_the_one_that_runs(self): + rules = self._register("detect_empty_thing") + assert rules["empty_thing"]["func"].__name__ == "detect_empty_thing" + + +class TestDetectGating: + """`detect` runs only the rules that apply, and only on finished samples.""" + + def test_a_non_final_sample_is_not_inspected(self): + # A sample still moving through the pipeline has no settled output to + # judge; flagging it would report anomalies that resolve themselves. + ctx = TaskContext(sample_id=0, raw_sample={}).to_preprocessed("pre") + assert TaskAnomalyDetector(root_dir=Path("/tmp")).detect(ctx, {"gen"}) == {} + + def test_a_failed_sample_is_not_inspected(self): + ctx = TaskContext(sample_id=0, raw_sample={}).to_preprocessed("pre") + ctx = ctx.to_failed(None, "error", "msg") + assert TaskAnomalyDetector(root_dir=Path("/tmp")).detect(ctx, {"gen"}) == {} + + def test_no_tags_means_no_detection(self): + # Rules select on tags; with none, every rule would either all-apply or + # none-apply, and neither is a defensible default. + ctx = _make_final_ctx(postprocess_result="answer") + assert TaskAnomalyDetector(root_dir=Path("/tmp")).detect(ctx, set()) == {} + + def test_rules_that_do_not_apply_are_skipped(self): + # A tag no rule declares must yield nothing rather than everything. + ctx = _make_final_ctx(postprocess_result="answer") + detector = TaskAnomalyDetector(root_dir=Path("/tmp")) + assert detector.detect(ctx, {"no_rule_declares_this_tag"}) == {} + + def test_a_rule_reporting_nothing_is_absent_from_the_result(self): + # Only tripped rules appear; an empty set per rule would make every + # sample look inspected-and-anomalous in the report's sample map. + ctx = _make_final_ctx(postprocess_result="a perfectly normal answer") + result = TaskAnomalyDetector(root_dir=Path("/tmp")).detect(ctx, {"gen"}) + assert all(indices for indices in result.values()) + + def test_has_anomalies_agrees_with_detect(self): + ctx = _make_final_ctx(postprocess_result="answer") + detector = TaskAnomalyDetector(root_dir=Path("/tmp")) + assert detector.has_anomalies(ctx, {"gen"}) is bool( + detector.detect(ctx, {"gen"}) + ) + assert detector.has_anomalies(ctx, set()) is False + + +class TestSaveIsAtomic: + """`save` writes a temp file and renames, so a crash never truncates. + + `anomalies.json` is read back by `load`/`needs_regeneration` on the next + run. A half-written file there is worse than none: it looks present, so the + report is not regenerated, and the run silently compares against garbage. + """ + + def _report(self, detector: TaskAnomalyDetector) -> AnomalyReport: + ctx = _make_final_ctx(postprocess_result="answer") + return detector.generate_report({0: ctx}, "t", task_tags={"gen"}) + + @pytest.mark.anyio + async def test_no_temp_file_is_left_behind(self, tmp_path): + detector = TaskAnomalyDetector(root_dir=tmp_path) + await detector.save(self._report(detector), backup_if_changed=False) + assert list(tmp_path.glob("*.tmp")) == [] + assert (tmp_path / "anomalies.json").exists() + + @pytest.mark.anyio + async def test_the_landing_file_is_not_the_temp_name(self, tmp_path): + # `with_suffix(None)` would drop the extension and write to `anomalies`, + # leaving `anomalies.json` stale forever while every save "succeeds". + detector = TaskAnomalyDetector(root_dir=tmp_path) + await detector.save(self._report(detector), backup_if_changed=False) + assert not (tmp_path / "anomalies").exists() + assert orjson.loads((tmp_path / "anomalies.json").read_bytes())["meta"] + + @pytest.mark.anyio + async def test_a_previous_report_is_replaced_not_appended(self, tmp_path): + detector = TaskAnomalyDetector(root_dir=tmp_path) + await detector.save(self._report(detector), backup_if_changed=False) + second = self._report(detector) + second["meta"]["task_name"] = "second" + await detector.save(second, backup_if_changed=False) + on_disk = orjson.loads((tmp_path / "anomalies.json").read_bytes()) + assert on_disk["meta"]["task_name"] == "second" + + @pytest.mark.anyio + async def test_save_caches_the_report_for_needs_regeneration(self, tmp_path): + # `needs_regeneration` answers from `_current_report`; leaving it unset + # makes every run regenerate, and leaving it stale makes none. + detector = TaskAnomalyDetector(root_dir=tmp_path) + assert detector.needs_regeneration() is True + await detector.save(self._report(detector), backup_if_changed=False) + assert detector.needs_regeneration() is False + + @pytest.mark.anyio + async def test_a_write_failure_leaves_no_temp_file(self, tmp_path): + # The directory does not exist, so the temp write raises; the failure is + # logged rather than raised, and nothing is left half-written. + source = TaskAnomalyDetector(root_dir=tmp_path) + report = self._report(source) + detector = TaskAnomalyDetector(root_dir=tmp_path / "missing") + await detector.save(report, backup_if_changed=False) # no raise + assert not (tmp_path / "missing").exists() + + @pytest.mark.anyio + async def test_backup_is_skipped_when_there_is_nothing_to_back_up(self, tmp_path): + # First save of a run: no prior file, so the backup path must not run. + detector = TaskAnomalyDetector(root_dir=tmp_path) + await detector.save(self._report(detector), backup_if_changed=True) + assert list(tmp_path.glob("anomalies.*.json")) == [] + + +class TestAggregationFromPrecomputedResults: + """`generate_and_save_from_results` keeps two counters over the same data. + + `anomaly_sample_details` counts *occurrences* (one per sample-iteration that + tripped a rule); `anomaly_rollout_details` counts *rollouts* (how many + indices tripped). They are easy to swap and a swap misreports how widespread + an anomaly is — which is the whole question the report answers. + """ + + def _results(self) -> dict: + return { + # two iterations, one rule each, different numbers of rollouts + "s1": {0: {"rule_a": [0, 1, 2]}, 1: {"rule_a": [0]}}, + # a second sample tripping a different rule + "s2": {0: {"rule_b": [5]}}, + # present but clean — must not be counted or persisted + "s3": {}, + } + + async def _report(self, tmp_path): + detector = TaskAnomalyDetector(root_dir=tmp_path) + return await detector.generate_and_save_from_results( + self._results(), + task_name="t", + total_samples=10, + final_count=9, + failed_count=1, + ) + + @pytest.mark.anyio + async def test_only_samples_with_anomalies_are_counted(self, tmp_path): + report = await self._report(tmp_path) + assert report["summary"]["anomaly_samples"] == 2 + + @pytest.mark.anyio + async def test_a_clean_sample_is_not_persisted(self, tmp_path): + # Writing an empty entry per clean sample would make the report scale + # with the run rather than with its anomalies. + report = await self._report(tmp_path) + assert set(report["samples"]) == {"s1", "s2"} + + @pytest.mark.anyio + async def test_sample_details_count_occurrences_not_rollouts(self, tmp_path): + # rule_a trips in two iterations of one sample -> 2, regardless of the + # 4 rollouts involved. + report = await self._report(tmp_path) + assert report["summary"]["anomaly_sample_details"] == { + "rule_a": 2, + "rule_b": 1, + } + + @pytest.mark.anyio + async def test_rollout_details_count_rollouts_not_occurrences(self, tmp_path): + # rule_a: 3 indices + 1 index = 4 rollouts. + report = await self._report(tmp_path) + assert report["summary"]["anomaly_rollout_details"] == { + "rule_a": 4, + "rule_b": 1, + } + + @pytest.mark.anyio + async def test_run_totals_pass_through_untouched(self, tmp_path): + report = await self._report(tmp_path) + summary = report["summary"] + assert summary["total_samples"] == 10 + assert summary["final_samples"] == 9 + assert summary["failed_samples"] == 1 + + @pytest.mark.anyio + async def test_iteration_keys_are_stringified_for_json(self, tmp_path): + # The report round-trips through JSON, where integer keys would come + # back as strings anyway — doing it here keeps in-memory and on-disk + # shapes identical. + report = await self._report(tmp_path) + assert set(report["samples"]["s1"]) == {"0", "1"} + + @pytest.mark.anyio + async def test_the_report_is_written_not_just_returned(self, tmp_path): + await self._report(tmp_path) + assert (tmp_path / "anomalies.json").exists() + + @pytest.mark.anyio + async def test_the_report_carries_the_current_rules_hash(self, tmp_path): + # This is what `needs_regeneration` compares against later. + report = await self._report(tmp_path) + assert report["meta"]["rules_hash"] == get_rules_hash() + assert report["meta"]["task_name"] == "t" + + +class TestRulesHashStability: + """`rules_hash` is what tells a reader the rule set moved under them.""" + + def test_the_hash_is_stable_across_calls(self): + assert get_rules_hash() == get_rules_hash() + + def test_the_hash_is_pinned_to_the_current_rule_set(self): + """Changing any rule's prose rotates `anomalies.json` fleet-wide. + + The hash is computed over the whole rules schema — names, descriptions, + rationales. So editing a description, not just adding a rule, invalidates + every stored report's comparison and triggers a backup-and-regenerate on + every run that resumes. That is a deliberate, visible event, not a + drive-by wording fix. + + If this test fails you changed the rule set. That is allowed — update the + value here in the same commit, so the rotation is in the diff rather than + discovered in production. + """ + assert get_rules_hash() == "33e3c4cf9491114b" + + def test_the_rule_set_is_the_expected_size(self): + # Guards the other direction: a rule silently dropped from the registry + # stops being detected, and nothing else would notice. + assert len(get_applied_rules()) == 5 + + def test_the_hash_is_short_and_hex(self): + # Persisted into every report and compared as a string; a change in + # width or alphabet silently invalidates every stored comparison. + h = get_rules_hash() + assert len(h) == 16 + assert all(c in "0123456789abcdef" for c in h) + @pytest.mark.anyio async def test_generate_report_includes_failed(self, tmp_path): detector = TaskAnomalyDetector(root_dir=tmp_path) diff --git a/tests/unit/core/tasks/test_meta.py b/tests/unit/core/tasks/test_meta.py index 52654fe9..f65f646b 100644 --- a/tests/unit/core/tasks/test_meta.py +++ b/tests/unit/core/tasks/test_meta.py @@ -26,6 +26,7 @@ get_task_run_identity, iter_task_metas, sieval_task, + task_meta_from_dict, task_meta_to_dict, ) from tests.conftest import ModuleIsolation @@ -1058,3 +1059,129 @@ def test_get_task_class_surfaces_nested_import_error(tmp_path): finally: sieval.tasks.__path__.remove(str(tmp_path)) sys.modules.pop("sieval.tasks.broken_task_for_test", None) + + +class TestTaskMetaRoundTrip: + """`task_meta_from_dict` is the reverse of `task_meta_to_dict`. + + It had no field-level tests: 84 mutants survived, meaning any field could be + read from the wrong key, dropped, or swapped with its neighbour and nothing + would notice. `meta/index.json` is how every consumer outside this process + learns what a task is, so a mis-mapped field is not a local error. + """ + + def _full(self) -> TaskMeta: + # Every field distinct, so a swap between any two is visible. + return TaskMeta( + name="a_name", + display_name="A Display Name", + description="a description", + dataset="a_dataset", + eval_mode=EvalMode.GEN, + n_shot=7, + tags=("t1", "t2"), + deps_group="a_group", + model_type="chat", + reference_impl=ReferenceImpl( + source="a_source", url="https://example.com/x", notes="a note" + ), + status="experimental", + ) + + def test_round_trip_preserves_every_field(self): + meta = self._full() + assert task_meta_from_dict(task_meta_to_dict(meta)) == meta + + def test_each_field_lands_in_its_own_slot(self): + # Round-trip equality alone cannot catch a *symmetric* swap, so the + # fields are also read back individually. + got = task_meta_from_dict(task_meta_to_dict(self._full())) + assert got.name == "a_name" + assert got.display_name == "A Display Name" + assert got.description == "a description" + assert got.dataset == "a_dataset" + assert got.eval_mode is EvalMode.GEN + assert got.n_shot == 7 + assert got.tags == ("t1", "t2") + assert got.deps_group == "a_group" + assert got.model_type == "chat" + assert got.status == "experimental" + + def test_reference_impl_survives_the_round_trip(self): + got = task_meta_from_dict(task_meta_to_dict(self._full())) + assert got.reference_impl is not None + assert got.reference_impl.source == "a_source" + assert got.reference_impl.url == "https://example.com/x" + assert got.reference_impl.notes == "a note" + + def test_eval_mode_comes_back_as_the_enum(self): + # The dict carries its raw value; left as a string, a consumer comparing + # against EvalMode would silently never match. + got = task_meta_from_dict(task_meta_to_dict(self._full())) + assert isinstance(got.eval_mode, EvalMode) + + def test_tags_come_back_as_a_tuple(self): + # TaskMeta is frozen+slots; a list would make it unhashable and mutable + # through the caller's own reference. + got = task_meta_from_dict(task_meta_to_dict(self._full())) + assert isinstance(got.tags, tuple) + + +class TestTaskMetaFromDictDefaults: + """Absent optional keys fall back to the documented defaults. + + `index.json` rows are release-authored and omit fields sitting at their + default, so a wrong default here silently rewrites what a task claims to be. + """ + + def _minimal(self) -> dict: + return { + "name": "n", + "display_name": "d", + "description": "desc", + "dataset": "ds", + "eval_mode": EvalMode.GEN.value, + } + + def test_n_shot_defaults_to_zero(self): + assert task_meta_from_dict(self._minimal()).n_shot == 0 + + def test_tags_default_to_empty(self): + assert task_meta_from_dict(self._minimal()).tags == () + + def test_optional_strings_default_to_none(self): + got = task_meta_from_dict(self._minimal()) + assert got.deps_group is None + assert got.model_type is None + + def test_status_defaults_to_stable(self): + # An omitted status must not downgrade a task — consumers gate on this. + assert task_meta_from_dict(self._minimal()).status == "stable" + + def test_absent_reference_impl_is_none(self): + assert task_meta_from_dict(self._minimal()).reference_impl is None + + def test_reference_impl_notes_default_to_empty(self): + payload = self._minimal() | { + "reference_impl": {"source": "s", "url": "https://example.com/u"} + } + ref = task_meta_from_dict(payload).reference_impl + assert ref is not None + assert ref.notes == "" + + def test_present_values_beat_the_defaults(self): + payload = self._minimal() | { + "n_shot": 5, + "tags": ["x"], + "deps_group": "g", + "model_type": "gen", + "status": "experimental", + } + got = task_meta_from_dict(payload) + assert (got.n_shot, got.tags, got.deps_group, got.model_type, got.status) == ( + 5, + ("x",), + "g", + "gen", + "experimental", + ) diff --git a/tests/unit/core/tasks/test_progress.py b/tests/unit/core/tasks/test_progress.py index 09f8ae7e..15b28484 100644 --- a/tests/unit/core/tasks/test_progress.py +++ b/tests/unit/core/tasks/test_progress.py @@ -402,3 +402,94 @@ def test_set_status_clears_existing_postfix_when_empty(self): assert fake_pbar.postfix_calls assert fake_pbar.postfix_calls[-1] == ({}, False) prog.close() + + +class TestPbarConstruction: + """What the bar is *built with*, not just whether it is built. + + The three gating conditions were covered; the arguments were not, and they + carry real behaviour — most of all `position`, which is how MultiTaskRunner + keeps one runner's bar from overwriting another's. + """ + + def _kwargs(self, **overrides) -> dict: + with ( + patch("sys.stderr.isatty", return_value=True), + patch("sieval.core.tasks.progress.tqdm") as tqdm_cls, + ): + TaskProgress( + total=overrides.pop("total", 42), + desc=overrides.pop("desc", "a-task"), + **overrides, + ).close() + return dict(tqdm_cls.call_args.kwargs) + + def test_total_and_desc_are_forwarded(self): + kwargs = self._kwargs(total=42, desc="a-task") + assert kwargs["total"] == 42 + assert kwargs["desc"] == "a-task" + + def test_position_is_forwarded(self): + # MultiTaskRunner passes progress_position=i; collapsing it to a + # constant makes concurrent runners draw over each other. + assert self._kwargs(position=3)["position"] == 3 + + def test_position_defaults_to_the_first_row(self): + assert self._kwargs()["position"] == 0 + + def test_the_bar_is_left_on_screen(self): + # leave=False would erase the final counts the moment a run finishes, + # which is the one moment they are worth reading. + assert self._kwargs()["leave"] is True + + def test_the_unit_is_samples(self): + assert self._kwargs()["unit"] == "sample" + + +class TestNonTtyLogFallback: + """`_enable_log` is exactly "show_progress and not a TTY".""" + + def _enabled(self, *, show_progress: bool, tty: bool) -> bool: + with patch("sys.stderr.isatty", return_value=tty): + prog = make_progress(show_progress=show_progress) + enabled = prog._enable_log + prog.close() + return enabled + + def test_enabled_only_off_tty(self): + assert self._enabled(show_progress=True, tty=False) is True + + def test_disabled_on_a_tty_where_the_bar_takes_over(self): + assert self._enabled(show_progress=True, tty=True) is False + + def test_disabled_when_progress_is_off_entirely(self): + # Silencing progress must silence both channels, not swap one for the + # other — otherwise `show_progress=False` still writes to the log. + assert self._enabled(show_progress=False, tty=False) is False + assert self._enabled(show_progress=False, tty=True) is False + + +class TestDumpGating: + """The dump file exists only when a directory *and* the flag are given.""" + + def _has_file(self, *, root_dir, dump_progress: bool) -> bool: + prog = make_progress(root_dir=root_dir, dump_progress=dump_progress) + has = prog._progress_file is not None + prog.close() + return has + + def test_both_required(self, tmp_path): + assert self._has_file(root_dir=tmp_path, dump_progress=True) is True + + def test_a_directory_alone_does_not_enable_it(self, tmp_path): + assert self._has_file(root_dir=tmp_path, dump_progress=False) is False + + def test_the_flag_alone_does_not_enable_it(self): + # No directory to write into; enabling here would raise mid-run rather + # than at construction. + assert self._has_file(root_dir=None, dump_progress=True) is False + + def test_the_file_is_named_progress_json(self, tmp_path): + prog = make_progress(root_dir=tmp_path, dump_progress=True) + assert prog._progress_file == tmp_path / "progress.json" + prog.close()