From c248a47f7e3214200c50eaa5b121da2b82b7abbc Mon Sep 17 00:00:00 2001 From: Gunnar Kreitz Date: Tue, 1 Sep 2026 09:53:08 +0200 Subject: [PATCH 1/4] Warn when declared score ranges on test groups seem too loose --- problemtools/checks/testdata.py | 108 ++++++++++++++++++++++++++++---- 1 file changed, 97 insertions(+), 11 deletions(-) diff --git a/problemtools/checks/testdata.py b/problemtools/checks/testdata.py index 84413468..b9f54f6d 100644 --- a/problemtools/checks/testdata.py +++ b/problemtools/checks/testdata.py @@ -6,6 +6,7 @@ import glob import hashlib import os +from collections.abc import Callable from pathlib import Path from ..context import Context @@ -47,6 +48,14 @@ def check_testdata( has_custom_grader = graders.grader is not None has_default_grader = DEFAULT_GRADER is not None + if metadata.is_scoring(): + # Whether the selected output validator might emit an arbitrary score via score.txt, + # making a test case's score unbounded as far as _check_score_range is concerned. + custom_scoring_possible = ( + not output_validators.uses_default(format_version, metadata) and metadata.is_custom_score_allowed() + ) + _check_score_range(testdata, custom_scoring_possible, diag) + input_validation = InputValidationCache(input_validators, work_dir) input_validation.precompute(testdata, context) @@ -64,6 +73,92 @@ def check_testdata( ) +#: Score aggregators for `grading: default`, matching support/default_grader's `score_aggregators`. +#: All are monotonic non-decreasing in each argument, which is what makes _check_score_range below +#: correct: the range of an aggregate is the aggregator applied to the children's lower bounds, and +#: separately to their upper bounds. +_SCORE_AGGREGATORS: dict[str, Callable[[list[float]], float]] = { + 'sum': sum, + 'avg': lambda scores: sum(scores) / len(scores), + 'min': min, + 'max': max, +} + + +def _check_score_range(group: TestDataGroup, custom_scoring_possible: bool, diag: Diagnostics) -> tuple[float, float]: + """Recursively check `group`'s declared score `range` against what can be inferred from its + grading configuration and test data. Returns group's effective range.""" + children = group.items + if group.is_root and 'ignore_sample' in group.config['grader_flags'].split(): + children = [child for child in children if not (isinstance(child, TestDataGroup) and child.datadir.name == 'sample')] + + if not children: + aggregate = (0.0, 0.0) + elif group.config['grading'] == 'custom': + # A custom grader can't be reasoned about. + aggregate = (float('-inf'), float('inf')) + else: + child_ranges = [] + for child in children: + if isinstance(child, TestDataGroup): + child_ranges.append(_check_score_range(child, custom_scoring_possible, diag)) + elif custom_scoring_possible: + child_ranges.append((float('-inf'), float('inf'))) + else: + accept_score, reject_score = group.config['accept_score'], group.config['reject_score'] + child_ranges.append((min(accept_score, reject_score), max(accept_score, reject_score))) + + aggregator_name = 'sum' + for flag in group.config['grader_flags'].split(): + if flag in _SCORE_AGGREGATORS: + aggregator_name = flag # last one wins, matching default_grader + aggregator = _SCORE_AGGREGATORS[aggregator_name] + + aggregate = (aggregator([lo for lo, _hi in child_ranges]), aggregator([hi for _lo, hi in child_ranges])) + + try: + score_range = group.config['range'] + min_score, max_score = list(map(float, score_range.split())) + if min_score > max_score: + diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") + return aggregate + except VerifyError: + raise + except Exception: + diag.error(f"Invalid format '{score_range}' for range: must be exactly two floats") + return aggregate + + agg_min, agg_max = aggregate + is_default_range = score_range == DEFAULT_CONFIG['range'] + if max_score < agg_min or min_score > agg_max: + diag.warning( + f"Declared score range '{score_range}' for {group} doesn't overlap with the computed range " + f'[{agg_min:g}, {agg_max:g}] at all' + ) + # We're in a bad state here, unclear what to return. Specified range probably ends up less spammy. + return (min_score, max_score) + elif min_score < agg_min or max_score > agg_max: + if is_default_range: + diag.warning( + f'No score range declared for {group}, but a range of [{agg_min:g}, {agg_max:g}] can be ' + f"computed from its grading configuration and test data; consider adding 'range: {agg_min:g} {agg_max:g}'" + ) + else: + diag.warning( + f"Declared score range '{score_range}' for {group} is looser than the computed range " + f'[{agg_min:g}, {agg_max:g}]; consider tightening it' + ) + elif group.is_root and is_default_range: + # The default range -inf, inf basically never makes sense. Encourage tighter even when we can't compute a recommendation + diag.warning( + f'No score range declared for {group}, and none could be computed automatically; as ' + 'the top-level group, its range is the overall score range for the problem -- consider ' + 'declaring one explicitly' + ) + + return (max(agg_min, min_score), min(agg_max, max_score)) + + def _check_group( group: TestDataGroup, context: Context, @@ -104,17 +199,8 @@ def _check_group( if group.config['on_reject'] not in ['break', 'continue']: diag.error(f"Invalid value '{group.config['on_reject']}' for on_reject policy") - if metadata.is_scoring(): - # Check grading - try: - score_range = group.config['range'] - min_score, max_score = list(map(float, score_range.split())) - if min_score > max_score: - diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") - except VerifyError: - raise - except Exception: - diag.error(f"Invalid format '{score_range}' for range: must be exactly two floats") + # Score range validity and tightness are checked by _check_score_range, called once for the + # whole tree from check_testdata. if group.is_root: seen_secret = False From cd3e1907ce2202bf6f7046abebcfbd66ad68509a Mon Sep 17 00:00:00 2001 From: Gunnar Kreitz Date: Tue, 1 Sep 2026 10:33:34 +0200 Subject: [PATCH 2/4] Add test coverage for score range warnings --- tests/conftest.py | 40 +++++++ tests/test_score_range.py | 237 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 277 insertions(+) create mode 100644 tests/conftest.py create mode 100644 tests/test_score_range.py diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..8dc9c443 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,40 @@ +"""Shared pytest fixtures and test doubles.""" + +import pytest + +from problemtools.diagnostics import Diagnostics + + +class RecordingDiagnostics(Diagnostics): + """A Diagnostics that records messages instead of emitting them, for asserting on in tests.""" + + def __init__(self) -> None: + self.messages: list[tuple[str, str]] = [] + + def error(self, msg: str, additional_info: str | None = None) -> None: + self.messages.append(('error', msg)) + + def warning(self, msg: str, additional_info: str | None = None) -> None: + self.messages.append(('warning', msg)) + + def info(self, msg: str) -> None: + pass + + def debug(self, msg: str) -> None: + pass + + def child(self, name: str) -> Diagnostics: + return self + + @property + def errors(self) -> int: + return len([m for m in self.messages if m[0] == 'error']) + + @property + def warnings(self) -> int: + return len([m for m in self.messages if m[0] == 'warning']) + + +@pytest.fixture +def diag() -> RecordingDiagnostics: + return RecordingDiagnostics() diff --git a/tests/test_score_range.py b/tests/test_score_range.py new file mode 100644 index 00000000..8361bf43 --- /dev/null +++ b/tests/test_score_range.py @@ -0,0 +1,237 @@ +from pathlib import Path + +from problemtools.checks.testdata import _check_score_range +from problemtools.model.testdata import TestCase, TestDataGroup + +# Not test classes -- just named that way by the model. Tell pytest not to collect them. +TestCase.__test__ = False +TestDataGroup.__test__ = False + +INF = float('inf') + + +def make_testcase(name: str) -> TestCase: + path = Path(name) + return TestCase( + infile=path.with_suffix('.in'), + ansfile=path.with_suffix('.ans'), + path=path, + input_validator_flags=[], + output_validator_flags=[], + ) + + +def make_group( + name: str, + items: list[TestCase | TestDataGroup], + grader_flags: str = '', + grading: str = 'default', + range_: str = '-inf +inf', + accept_score: float = 1.0, + reject_score: float = 0.0, + is_root: bool = False, +) -> TestDataGroup: + return TestDataGroup( + name=name, + datadir=Path(name), + config={ + 'grading': grading, + 'grader_flags': grader_flags, + 'range': range_, + 'accept_score': accept_score, + 'reject_score': reject_score, + }, + is_root=is_root, + items=items, + ) + + +def test_group_sum_is_default_aggregator(diag): + group = make_group( + 'g', [make_testcase('a'), make_testcase('b'), make_testcase('c')], range_='0 30', accept_score=10, reject_score=0 + ) + assert _check_score_range(group, False, diag) == (0, 30) + assert diag.messages == [] + + +def test_group_avg_aggregator(diag): + group = make_group('g', [make_testcase('a')], grader_flags='avg', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + + +def test_reject_above_accept(diag): + # Corner case: check we don't end up with a broken range if reject_score is above accept_score + group = make_group('g', [make_testcase('a')], accept_score=0, reject_score=5) + assert _check_score_range(group, False, diag) == (0, 5) + + +def test_custom_scoring_possible_is_unbounded(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0) + assert _check_score_range(group, True, diag) == (-INF, INF) + + +def test_group_min_aggregator(diag): + group = make_group( + 'g', + [make_testcase('a'), make_testcase('b')], + grader_flags='min', + ) + # accept_score/reject_score are shared across a group's direct testcase children (1.0/0.0 here). + assert _check_score_range(group, False, diag) == (0, 1) + + +def test_group_max_aggregator(diag): + group = make_group('g', [make_testcase('a'), make_testcase('b')], grader_flags='max') + assert _check_score_range(group, False, diag) == (0, 1) + + +def test_last_aggregator_flag_wins(diag): + group = make_group('g', [make_testcase('a')], grader_flags='max min avg', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + + +def test_custom_grading_is_unbounded(diag): + group = make_group('g', [make_testcase('a')], grading='custom') + assert _check_score_range(group, False, diag) == (-INF, INF) + + +def test_empty_group_scores_zero(diag): + group = make_group('g', []) + assert _check_score_range(group, False, diag) == (0, 0) + + +def test_nested_groups_compose(diag): + subtask1 = make_group( + 'g.subtask1', + [make_testcase('a'), make_testcase('b')], + grader_flags='min', + range_='0 50', + accept_score=50, + reject_score=0, + ) + subtask2 = make_group('g.subtask2', [make_testcase('c')], grader_flags='min', range_='0 50', accept_score=50, reject_score=0) + secret = make_group('g.secret', [subtask1, subtask2], range_='0 100') + assert _check_score_range(secret, False, diag) == (0, 100) + assert diag.messages == [] + + +def test_ignore_sample_at_root_skips_sample_group(diag): + sample = make_group('sample', [make_testcase('s')], accept_score=1000, reject_score=0) + secret = make_group('secret', [make_testcase('a')], accept_score=100, reject_score=0) + root = make_group('data', [sample, secret], grader_flags='ignore_sample', is_root=True) + assert _check_score_range(root, False, diag) == (0, 100) + + +def test_ignore_sample_is_a_no_op_below_root(diag): + sample = make_group('sample', [make_testcase('s')], accept_score=1000, reject_score=0) + secret = make_group('secret', [make_testcase('a')], accept_score=100, reject_score=0) + # Misconfigured (checks._check_group flags this separately). We just aggregate all children + non_root = make_group('g', [sample, secret], grader_flags='ignore_sample', is_root=False) + assert _check_score_range(non_root, False, diag) == (0, 1100) + + +# --- Declared range vs. what can be inferred --- + + +def test_no_warning_when_declared_matches_computed(diag): + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [] + + +def test_looser_declared_range_warns_and_suggests_tightening(diag): + group = make_group('g', [make_testcase('a')], range_='0 100', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + "Declared score range '0 100' for testcase group g is looser than the computed range [0, 10]; consider tightening it", + ) + ] + + +def test_no_declared_range_warns_and_suggests_one(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0) # range left at the default + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, but a range of [0, 10] can be computed from its ' + "grading configuration and test data; consider adding 'range: 0 10'" + ), + ) + ] + + +def test_narrower_declared_range_is_trusted_without_warning(diag): + # A "bad guarantee": the group's own children can clearly reach 100, but the author declared a + # narrower range. We don't warn -- that's a promise checked elsewhere (checks.submissions) -- + # but we do trust it for the returned (propagated) value. + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [] + + +def test_narrower_declared_range_propagates_to_parent(diag): + # The parent's aggregate must reflect the child's effective (trusted) range, not its raw + # aggregate -- so a narrow declaration deep in the tree is reflected in ancestors' results too. + bad_child = make_group('g.secret', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=0) + root = make_group('g', [bad_child], is_root=True) + assert _check_score_range(root, False, diag) == (0, 10) + # No warning at the child, but the root's default range is loose + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, but a range of [0, 10] can be computed from its ' + "grading configuration and test data; consider adding 'range: 0 10'" + ), + ) + ] + + +def test_disjoint_declared_range_warns_distinctly(diag): + group = make_group('g', [make_testcase('a')], range_='0 10', accept_score=100, reject_score=50) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.messages == [ + ( + 'warning', + "Declared score range '0 10' for testcase group g doesn't overlap with the computed range [50, 100] at all", + ) + ] + + +def test_root_with_unbounded_aggregate_and_no_declared_range_still_warns(diag): + group = make_group('g', [make_testcase('a')], grading='custom', is_root=True) + assert _check_score_range(group, False, diag) == (-INF, INF) + assert diag.messages == [ + ( + 'warning', + ( + 'No score range declared for testcase group g, and none could be computed automatically; as the ' + 'top-level group, its range is the overall score range for the problem -- consider declaring one ' + 'explicitly' + ), + ) + ] + + +def test_non_root_with_unbounded_aggregate_and_no_declared_range_is_silent(diag): + group = make_group('g', [make_testcase('a')], grading='custom', is_root=False) + assert _check_score_range(group, False, diag) == (-INF, INF) + assert diag.messages == [] + + +def test_invalid_range_format_errors_and_falls_back_to_aggregate(diag): + group = make_group('g', [make_testcase('a')], range_='not a range', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.errors == 1 + assert "Invalid format 'not a range'" in diag.messages[0][1] + + +def test_min_greater_than_max_errors_and_falls_back_to_aggregate(diag): + group = make_group('g', [make_testcase('a')], range_='10 0', accept_score=10, reject_score=0) + assert _check_score_range(group, False, diag) == (0, 10) + assert diag.errors == 1 + assert 'cannot be greater than maximum score' in diag.messages[0][1] From 7b719f3525d6712e2f9efd94810d83fabed04fbf Mon Sep 17 00:00:00 2001 From: Gunnar Kreitz Date: Tue, 1 Sep 2026 11:09:25 +0200 Subject: [PATCH 3/4] Add checks for negative score range and reject_score usage --- problemtools/checks/testdata.py | 30 +++++++++++++++++++++++++++++- tests/test_score_range.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/problemtools/checks/testdata.py b/problemtools/checks/testdata.py index b9f54f6d..79bfa264 100644 --- a/problemtools/checks/testdata.py +++ b/problemtools/checks/testdata.py @@ -6,7 +6,7 @@ import glob import hashlib import os -from collections.abc import Callable +from collections.abc import Callable, Iterator from pathlib import Path from ..context import Context @@ -49,6 +49,8 @@ def check_testdata( has_default_grader = DEFAULT_GRADER is not None if metadata.is_scoring(): + _warn_reject_score(testdata, diag) + # Whether the selected output validator might emit an arbitrary score via score.txt, # making a test case's score unbounded as far as _check_score_range is concerned. custom_scoring_possible = ( @@ -73,6 +75,27 @@ def check_testdata( ) +def _all_groups(group: TestDataGroup) -> Iterator[TestDataGroup]: + """`group` and all its descendant groups.""" + yield group + for subgroup in group.get_subgroups(): + yield from _all_groups(subgroup) + + +def _warn_reject_score(testdata: TestDataGroup, diag: Diagnostics) -> None: + """Warn about reject_score usage.""" + groups = list(_all_groups(testdata)) + + nonzero_reject = [(g, g.config['reject_score']) for g in groups if g.config['reject_score'] != 0] + if nonzero_reject: + example_group, example_score = nonzero_reject[0] + diag.warning( + f'{len(nonzero_reject)} testcase group(s) configure a non-zero reject_score (e.g. {example_group} ' + f'has reject_score {example_score:g}); submissions with non-AC final verdict always have score 0, ' + 'so this is usually a mistake' + ) + + #: Score aggregators for `grading: default`, matching support/default_grader's `score_aggregators`. #: All are monotonic non-decreasing in each argument, which is what makes _check_score_range below #: correct: the range of an aggregate is the aggregator applied to the children's lower bounds, and @@ -155,6 +178,11 @@ def _check_score_range(group: TestDataGroup, custom_scoring_possible: bool, diag 'the top-level group, its range is the overall score range for the problem -- consider ' 'declaring one explicitly' ) + elif group.is_root and min_score < 0: + diag.warning( + f"Declared score range '{score_range}' for {group} has a negative minimum; submissions with " + 'non-AC final verdict always have score 0, so a negative minimum is usually a mistake' + ) return (max(agg_min, min_score), min(agg_max, max_score)) diff --git a/tests/test_score_range.py b/tests/test_score_range.py index 8361bf43..2dcaf47c 100644 --- a/tests/test_score_range.py +++ b/tests/test_score_range.py @@ -223,6 +223,36 @@ def test_non_root_with_unbounded_aggregate_and_no_declared_range_is_silent(diag) assert diag.messages == [] +def test_root_with_negative_minimum_and_no_other_issue_warns(diag): + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=-5, range_='-5 10', is_root=True) + assert _check_score_range(group, False, diag) == (-5, 10) + assert diag.messages == [ + ( + 'warning', + ( + "Declared score range '-5 10' for testcase group g has a negative minimum; submissions with " + 'non-AC final verdict always have score 0, so a negative minimum is usually a mistake' + ), + ) + ] + + +def test_negative_minimum_below_root_is_not_flagged(diag): + # negative scores for non-root test groups is a bit weird, but IMHO not weird enough to warn about + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=-5, range_='-5 10', is_root=False) + assert _check_score_range(group, False, diag) == (-5, 10) + assert diag.messages == [] + + +def test_negative_minimum_at_root_is_suppressed_by_other_warnings(diag): + # Warning about a negative range is low priority. If we can instead suggest to narrow the range, + # that's typically a better warning. + group = make_group('g', [make_testcase('a')], accept_score=10, reject_score=0, range_='-10 200', is_root=True) + assert _check_score_range(group, False, diag) == (0, 10) + assert len(diag.messages) == 1 + assert 'looser than the computed range' in diag.messages[0][1] + + def test_invalid_range_format_errors_and_falls_back_to_aggregate(diag): group = make_group('g', [make_testcase('a')], range_='not a range', accept_score=10, reject_score=0) assert _check_score_range(group, False, diag) == (0, 10) From f770706507f84421f3156cd8b9486afbb104e406 Mon Sep 17 00:00:00 2001 From: Gunnar Kreitz Date: Tue, 1 Sep 2026 11:54:37 +0200 Subject: [PATCH 4/4] Simplify _check_score_range a bit --- problemtools/checks/testdata.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/problemtools/checks/testdata.py b/problemtools/checks/testdata.py index 79bfa264..2a1490fc 100644 --- a/problemtools/checks/testdata.py +++ b/problemtools/checks/testdata.py @@ -10,7 +10,7 @@ from pathlib import Path from ..context import Context -from ..diagnostics import Diagnostics, VerifyError +from ..diagnostics import Diagnostics from ..formatversion import FormatVersion from ..judge import validate_output from ..metadata import Metadata @@ -139,19 +139,28 @@ def _check_score_range(group: TestDataGroup, custom_scoring_possible: bool, diag aggregate = (aggregator([lo for lo, _hi in child_ranges]), aggregator([hi for _lo, hi in child_ranges])) + score_range = group.config['range'] try: - score_range = group.config['range'] min_score, max_score = list(map(float, score_range.split())) - if min_score > max_score: - diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") - return aggregate - except VerifyError: - raise except Exception: diag.error(f"Invalid format '{score_range}' for range: must be exactly two floats") return aggregate + if min_score > max_score: + diag.error(f"Invalid score range '{score_range}': minimum score cannot be greater than maximum score") + return aggregate + + return _warn_score_range(group, (min_score, max_score), aggregate, diag) + + +def _warn_score_range( + group: TestDataGroup, declared: tuple[float, float], aggregate: tuple[float, float], diag: Diagnostics +) -> tuple[float, float]: + """Compare `group`'s declared score range to what its `aggregate` says can actually be achieved, + warn about any mismatch, and return the effective (declared-trusting) range.""" + min_score, max_score = declared agg_min, agg_max = aggregate + score_range = group.config['range'] is_default_range = score_range == DEFAULT_CONFIG['range'] if max_score < agg_min or min_score > agg_max: diag.warning(