Opened 3 years ago

Last modified 3 years ago

#22745 new task

Establish policy for writing regression tests

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: policy, docs, 034-triage-20180328, 034-removed-20180328
Cc: mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Ideally, every non-trivial bug fix will have a regression test. Consider making and documenting a policy that requires that bug fixes will have accompanying regression tests, or some searchable justification (Trac keyword?) for why it's infeasible to add one. (Often this is a symptom that the underlying code is too complex and needs refactoring.)

This documentation should probably be somewhere under doc/HACKING.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by catalyst

After discussion on IRC, it seems like test-waiver might be a reasonable keyword to use to mark tickets where we decide that it's infeasible to add a regression test for a bug.

Such a ticket should contain a written justification for why it's infeasible to add a regression test covering the bug. This can include excessive code complexity or the requirement for external test fixtures that need to be manually set up.

Being able to easily search Trac for such tickets will make it easier to locate places where we could improve the testability of our code.

comment:2 Changed 3 years ago by catalyst

Also patch reviewers should make sure the patch contains a regression test, and if one isn't present, make sure that there's a test-waiver keyword along with a comment explaining why that patch should be an exception from this rule. The reviewer should also review the justification and decide whether it's reasonable.

comment:3 Changed 3 years ago by teor

Type: defecttask

Do we also have a policy requiring any new features to have unit tests?

I think we should make it consistent across all tickets, and use the test-waiver keyword when we merge any code without tests. (Then we can search for "defect" or "regression" and "test-waiver" to find defect waivers, or regression waivers.)

Also, let's start trialing this for (some?) new code/tickets straight away, so we work out the bugs in the process.

comment:4 Changed 3 years ago by catalyst

I think already requires unit tests for new functionality. Also, I think we should be really strict about allowing waivers for testing of new functionality. I guess there are some features where pre-existing technical debt makes it really hard to add tests for the new stuff, but in those cases I would argue strongly for paying down the technical debt prior to implementing the new features.

comment:5 Changed 3 years ago by mcs

Cc: mcs added

comment:6 Changed 3 years ago by nickm

Keywords: policy docs added

comment:7 Changed 3 years ago by nickm

Keywords: policy docspolicy, docs

add the "docs" keyword to tickets that are documentation-only, and can enter Tor post-freeze.

comment:8 Changed 3 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

comment:9 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Moving a bunch of tickets from 033 to 034.

comment:10 Changed 3 years ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 3 years ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:12 Changed 3 years ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

Note: See TracTickets for help on using tickets.