Skip to content

Conversation

@sethmlarson
Copy link
Contributor

@sethmlarson sethmlarson commented Jan 16, 2026

@sethmlarson sethmlarson added type-security A security issue needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jan 16, 2026
@sethmlarson sethmlarson requested a review from gpshead January 16, 2026 17:08
Comment on lines +509 to +513
self.assertRaises(ValueError, headers.__setitem__, f"key{c0}", "val")
self.assertRaises(ValueError, headers.__setitem__, "key", f"val{c0}")
self.assertRaises(ValueError, headers.add_header, f"key{c0}", "val", param="param")
self.assertRaises(ValueError, headers.add_header, "key", f"val{c0}", param="param")
self.assertRaises(ValueError, headers.add_header, "key", "val", param=f"param{c0}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider marking params as subtests for better test report.

Suggested change
self.assertRaises(ValueError, headers.__setitem__, f"key{c0}", "val")
self.assertRaises(ValueError, headers.__setitem__, "key", f"val{c0}")
self.assertRaises(ValueError, headers.add_header, f"key{c0}", "val", param="param")
self.assertRaises(ValueError, headers.add_header, "key", f"val{c0}", param="param")
self.assertRaises(ValueError, headers.add_header, "key", "val", param=f"param{c0}")
with self.subTest(control_character=hex(ord(c0[0]))):
self.assertRaises(ValueError, headers.__setitem__, f"key{c0}", "val")
self.assertRaises(ValueError, headers.__setitem__, "key", f"val{c0}")
self.assertRaises(ValueError, headers.add_header, f"key{c0}", "val", param="param")
self.assertRaises(ValueError, headers.add_header, "key", f"val{c0}", param="param")
self.assertRaises(ValueError, headers.add_header, "key", "val", param=f"param{c0}")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you okay if I don't adopt subTest() here? I ask because I have many other PRs out that I'd need to modify, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I won't insist. It's just that I usually try to make logical tests obvious. It's typically easier in newly added lines rather than when refactoring existing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes type-security A security issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants