Skip to content

Conversation

rongxin-liu
Copy link
Contributor

@rongxin-liu rongxin-liu commented Jul 23, 2025

This PR introduces several improvements and new features to the check50 codebase, focusing on better assertion handling and error reporting.

Assertion Handling Improvements:

  • Introduced the check50/assertions/rewrite.py module, which rewrites assert statements into check50_assert calls. This allows for custom exception handling and better integration with check50's error reporting.
  • Added the check50/assertions/runtime.py module, defining the check50_assert function. This function evaluates conditions, raises appropriate exceptions (Mismatch, Missing, or Failure), and provides detailed context for failed assertions.
  • Integrated the rewrite and rewrite_enabled functions into the main execution flow in check50/__main__.py. If the rewrite feature is enabled, assert statements in check files are replaced with check50_assert calls.

Error Reporting Improvements:

  • Enhanced the Mismatch exception in check50/_api.py to provide more detailed and user-friendly error messages, including truncation logic for long outputs.
  • Introduced a _truncate helper function in check50/_api.py to dynamically truncate strings based on differences with other strings or fixed limits. This improves readability of error messages.
  • International Language Support

Assert statements will now raise a check50.Failure by default
@rongxin-liu rongxin-liu marked this pull request as ready for review August 1, 2025 19:08
@rongxin-liu rongxin-liu added this to the 4.0.0 milestone Aug 1, 2025
ivanharvard and others added 22 commits August 1, 2025 17:37
…t; in which builtins were overwritten by context; added tokenization of vars
--assertion-rewrite as an optional flag to override ENABLE_CHECK50_ASSERT
Serialize expected and actual values correctly
Added _process_list as a _raw and _truncate helper
@cmlsharp
Copy link
Contributor

cmlsharp commented Aug 31, 2025

The assertion rewriting is a really cool idea! Cool to see this still being developed. Some comments:

  • Since the minimum python version for this version of check50 appears to be >= 3.10 (per the match statements), check50/contextmanagers.py could be deleted. It contains an implementation of contextlib.nullcontext which was 3.7+ and I think we wanted to support Python 3.6 at the time.

  • Out of curiosity, how does the assertion rewriter intersect with check50.import_checks? Do imported files get rewritten as well? Just looking at the code it seems like no, which seems highly unintuitive (e.g. if mario/less utilized asserts and had rewriting enabled, mario/more which imports the checks from less would not have its assertions rewritten, and so you'd get errors instead of failures on failed asserts. Am I showing my age with these pset names?).

    Even if I'm not importing checks from another directory, if I split my checks within a directory up into multiple files and then import them into my __init__.py, do the assertions there get rewritten? I think similarly the answer is "no". That case is probably not as important to cover since the common case probably is all checks in one file, but still. Obviously you wouldn't want to try to recurse over every import in __init__.py but perhaps some generalization of check50.import_checks could cover this case? (e.g. foo = check50.import_checks("./foo.py") would import foo.py and call the assertion rewriter if appropriate. Right now, import_checks I think is specialized to directories.)

    Edit: This is still annoyingly fragile since someone might (very sensibly) expect normal pythonic imports to Just Work. Maybe a limited recursion exclusively over imports within the package could cover this case. Meanwhile simply updating check50.import_checks to run the assertion rewriter on the module it imports should suffice to cover importing checks from another directory.

  • Edit2: this shouldn't block merging, but at some point it might be worth considering whether the attrs dependency can be removed. Since this was originally implemented, Python now has dataclasses in the standard library which are a simplified version of attrs. If the more advanced features of attrs aren't being used, then the dependency might not be needed at all.

This was referenced Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Issues relating to check50 4.x enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants