-
Notifications
You must be signed in to change notification settings - Fork 56
docs: harvest review lessons from PRs reviewed 2026-07-31 to 2026-08-06 #10145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -240,12 +240,10 @@ SCHEMA_VALIDATION_TEST_CASES: list[SchemaValidationTestCase] = [ | |||||
|
|
||||||
| ### When to Use This Pattern | ||||||
|
|
||||||
| Use the dataclass test case pattern when: | ||||||
| Reach for the dataclass test case pattern when several scenarios share one structure and only the data | ||||||
| varies — it keeps the pytest ids readable as the list grows. | ||||||
|
|
||||||
| - Testing a function with multiple input/output scenarios | ||||||
| - Test cases share a common structure | ||||||
| - You want readable test IDs in pytest output | ||||||
| - The test logic is the same but data varies | ||||||
| Whichever you pick, the case data lives in the `parametrize` decorator. Don't parametrize over the keys of a module-level dict and look the values up inside the test — the reader has to hold two places in their head to see what a case actually asserts, and the pytest id no longer tells them. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The new guidance "the case data lives in the Prompt for AI agents
Suggested change
|
||||||
|
|
||||||
| For simpler cases with only 2-3 scenarios, standard `@pytest.mark.parametrize` with tuples may be sufficient: | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: This recommendation can undo the exception it extends:
tasks/__init__.pyeagerly imports every task submodule, so a top-level import of the new module in a task would reload the full backend on everyinvoke— the exact eager-load the exception avoids. The new module should keep heavy imports deferred (or the task should import it lazily), otherwise the 'deferred imports mostly disappear' advice is misleading when followed literally. Consider clarifying where the single import lives so the guidance stays consistent with the stated rationale.Prompt for AI agents