feat: forward t.Logf() output to testing.T on property test failures

[james-w]

Property tests now capture logs from t.Logf() and include them in the
error output when a test fails. This gives users debugging capability
while avoiding log spam from successful iterations.

Changes:

• Add testingT field and logs slice to specta.T struct
• Modify T.Logf() to capture messages instead of being a no-op
• Pass outer testing.T when creating T instances in Property()
• Forward captured logs in reportFailure() for shrunk counterexample
• Add t.Helper() to Draw() function for better stack traces
• Add comprehensive tests for log forwarding behavior

Logs are only shown for the final shrunk failing iteration, which
provides useful debugging context without cluttering test output.

[claude]

Code Review

I've reviewed this PR and the implementation looks solid overall. Here are my findings:

✅ Positives

1. Clean design: The logPassthrough flag elegantly solves the problem of forwarding logs only for the final failing iteration
2. Correct line attribution: Using Helper() ensures logs point to user code, not framework internals
3. Comprehensive test coverage: The new test file covers all important scenarios (success, failure, shrinking, multiple logs)
4. Good documentation: Inline comments clearly explain the mechanism and reasoning

🔍 Code Quality

property.go:20 - The logs []string field is now unused and can be removed:

logs           []string  // Captured log messages (unused when logPassthrough is true)

Since you're always using passthrough mode for the final replay and discarding logs otherwise, this field serves no purpose.

property.go:64-65 - Type assertion could use a helper variable for clarity:

if helper, ok := t.testingT.(interface{ Helper() }); ok {
    helper.Helper()
}

Minor: the interface could be extracted to a type for reusability (it's used in 3 places), but not critical.

property.go:274-276 - The warning for non-deterministic tests is good defensive programming. Consider if there are legitimate cases where this could trigger false positives (e.g., time-based generators without seeds).

🤔 Potential Issues

generator.go:29 - Adding t.Helper() to Draw() is correct for line attribution in property tests, but verify this doesn't affect error messages when Draw() is used outside property tests (e.g., in factory code).

Test determinism: property_logf_test.go uses fixed seeds which is good for reproducibility, but confirm the shrinking behavior is deterministic enough that tests won't flake.

Performance & Security

No concerns. The change is minimal overhead (one boolean flag, one conditional check per log call).

Summary

This is well-implemented functionality that provides genuine debugging value. The only actionable feedback is removing the unused logs field. Everything else is working as intended.

Recommendation: Approve with minor cleanup suggested.

[claude]

Code Review

Overall this is a well-implemented feature with clean design. I have a few observations:

Issues

property.go:20 - The logs []string field is now unused and should be removed:

logs           []string                   // Captured log messages (unused when logPassthrough is true)

Since you're always using passthrough mode for the final replay and discarding logs otherwise, this field serves no purpose.

Observations

property.go:64-65 - The type assertion for Helper() could be cleaner with an extracted helper variable, though this is used in 3 places so extracting it to a named type might help with consistency. Not critical.

generator.go:29 - Adding t.Helper() to Draw() is correct for property tests, but worth verifying it doesn't affect error messages when Draw() is used in other contexts (e.g., factory code).

property.go:274-276 - The non-deterministic warning is good defensive programming. Consider whether legitimate cases might trigger false positives (time-based generators without explicit seeds).

Test Coverage

The test coverage in property_logf_test.go is comprehensive - covers success, failure, shrinking, and ordering. Well done.

Summary

Clean implementation that provides genuine value. Only actionable item is removing the unused logs field. Everything else works as intended.

[claude]

Additional Review Notes

Two previous reviews already covered the main points well. One actionable item to address:

property.go:20 - Remove the unused logs []string field. The comment itself admits it's unused, and with the passthrough mechanism, logs are never captured to this slice.

The rest looks good - the implementation is clean, test coverage is solid, and the passthrough design avoids memory overhead from capturing/discarding logs on successful iterations.

[claude]

Review Summary

The previous reviews have correctly identified the main issue - the unused logs []string field at property.go:20 should be removed.

Beyond that, the implementation is solid:

• Clean passthrough design with correct line attribution via Helper()
• Comprehensive test coverage in property_logf_test.go
• Good defensive check for non-deterministic replay failures
• Minimal performance overhead

Recommendation: Remove the unused field and this is ready to merge.

[claude]

Review

The existing review comments have thoroughly covered this PR. I'll only add:

Action Required:

• property.go:20 - Remove the unused logs []string field (as noted in previous comments)

Everything Else Looks Good:

• Clean implementation using logPassthrough flag
• Proper use of Helper() for correct line attribution
• Comprehensive test coverage in property_logf_test.go
• Good defensive check for non-deterministic replay (property.go:266-268)

The implementation is solid and ready to merge once the unused field is removed.

[claude]

Review Summary

Multiple reviews have already identified the key issue:

Action Required:

• Remove the unused logs []string field at property.go:20 (the comment even acknowledges it's unused)

Otherwise looks good:

• Clean passthrough design with correct Helper() usage for line attribution
• Comprehensive test coverage
• Minimal overhead
• Good defensive checks

Ready to merge after removing the unused field.

[claude]

Code Review

This PR has been significantly improved since the earlier reviews. The unused logs field has been removed and the implementation is now clean.

✅ Strengths

Clean passthrough design (property.go:20, 56-67): The logPassthrough flag elegantly ensures logs are only forwarded on the final shrunk replay, avoiding spam from successful iterations while providing debugging value on failures.

Correct line attribution (property.go:63, generator.go:29): Using Helper() on both the testing.T instance and the Draw function ensures log messages and panics point to user code, not framework internals. Good attention to detail.

Comprehensive test coverage (property_logf_test.go): The test suite covers the key scenarios - success (no logs), failure with shrinking (logs show shrunk values), and multiple logs in order. Well-designed tests.

Defensive check (property.go:265-267): The sanity check for non-deterministic behavior is good defensive programming that could help catch subtle bugs.

Minor Observations

Type assertion pattern (property.go:63): The inline type assertion for Helper() is used in multiple places. While functional, extracting to a named interface type could improve consistency and readability. Not critical.

Draw Helper usage (generator.go:29): Adding t.Helper() to Draw() is correct for property tests. Worth verifying this doesn't affect error messages in other contexts (factory code, etc.), though the impact should be positive.

Summary

This is a well-implemented feature that provides genuine debugging value. The code is clean, well-tested, and follows Go best practices. The earlier feedback has been addressed.

Recommendation: Approve and merge.

[claude]

Code Review

The implementation looks clean and well-tested. The unused logs field mentioned in earlier reviews has been removed.

Key Strengths

• Clean passthrough design: Logs only forwarded on final shrunk replay (property.go:58-67)
• Correct line attribution: Proper use of Helper() at property.go:63 and generator.go:29
• Comprehensive tests: property_logf_test.go covers success, failure, shrinking, and ordering

Minor Note

The defensive check at property.go:265-267 for non-deterministic replay is good practice. Just be aware it could theoretically trigger false positives with time-based generators without explicit seeds, though unlikely in practice.

Recommendation: Approve and merge. The feature provides genuine debugging value without introducing technical debt.