Opened 18 months ago

Last modified 8 months 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:

Description

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 18 months 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 18 months 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 18 months 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 18 months ago by catalyst

I think CodingStandards.md 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 18 months ago by mcs

Cc: mcs added

comment:6 Changed 15 months ago by nickm

Keywords: policy docs added

comment:7 Changed 15 months 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 14 months ago by nickm

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

comment:9 Changed 11 months 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 9 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 9 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:12 Changed 8 months 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.