Opened 10 months ago

Closed 7 months ago

Last modified 6 months ago

#28668 closed enhancement (fixed)

If a Tor unit test causes a BUG log, it should fail

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, postfreeze-ok, 035-backport
Cc: Actual Points: .1
Parent ID: Points:
Reviewer: juga,asn Sponsor:

Description

In #28660, a successful unit test logged a BUG warning, but the test still succeeded.

We should make tests fail by default if they log a BUG() or LD_BUG warning, but allow some tests to expect bug warnings.

I thought we made this change, or had a ticket for this change, but I couldn't find it.

Child Tickets

TicketTypeStatusOwnerSummary
#28660defectclosedrend_cache_decrement_allocation(): Bug: Underflow in rend_cache_decrement_allocation (On Windows build)

Change History (16)

comment:1 Changed 10 months ago by nickm

I think #19999 was the ticket

comment:2 Changed 10 months ago by teor

In that ticket, we fixed all the bug warnings, but *didn't* make tests fail on BUG().

This time, we should fix all the bug warnings, and make the tests fail on BUG().

comment:3 in reply to:  2 Changed 10 months ago by dgoulet

Replying to teor:

In that ticket, we fixed all the bug warnings, but *didn't* make tests fail on BUG().

This time, we should fix all the bug warnings, and make the tests fail on BUG().

+1.

(Just to be clear, BUG() _and_ log_warn(LD_BUG, ...) should fail unless we expect them). #28660 had a LD_BUG since 0.2.8 that we never noticed :S... (was too early for me... BUG() is an LD_BUG :P).

Last edited 10 months ago by dgoulet (previous) (diff)

comment:4 Changed 8 months ago by nickm

Keywords: postfreeze-ok added

Mark some tickets as postfreeze-ok, to indicate that I think they are okay to accept in 0.4.0 post-freeze. Does not indicate that they are all necessary to do postfreeze.

comment:5 Changed 8 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:6 Changed 8 months ago by nickm

Actual Points: .1
Status: acceptedneeds_review

I've made a branch here as ticket28668_035, with an 0.3.5 PR as https://github.com/torproject/tor/pull/650 .

There is also an 0.4.0 branch as ticket28668_040 with a PR as https://github.com/torproject/tor/pull/651 . It fixes the LOG_ERR instance in test_pt.c as well.

comment:7 Changed 8 months ago by dgoulet

Reviewer: juga

comment:8 in reply to:  6 Changed 8 months ago by juga

Status: needs_reviewneeds_revision

Replying to nickm:

There is also an 0.4.0 branch as ticket28668_040 with a PR as https://github.com/torproject/tor/pull/651 . It fixes the LOG_ERR instance in test_pt.c as well.

In the 0.4.0 branch travis fails with an error and appveyor with other 2 different ones. Don't know if we can solve them as part of this ticket or should open new ones.

comment:9 Changed 8 months ago by nickm

I've updated both branches, but I'm going to wait until appveyor is done before I put this back in needs_review.

comment:10 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

okay; the CI is passing! Back into needs_review with this branch.

comment:11 Changed 7 months ago by juga

It looks good to me, but i'm not familiar with the functions used. It'd be great if somebody else can review it too.
Just left a comment about a commit message.

comment:12 Changed 7 months ago by asn

Reviewer: jugajuga,asn

comment:13 Changed 7 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:14 Changed 7 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:15 Changed 7 months ago by teor

Keywords: 035-backport added

We backported this change to 0.3.5.8, but no further.

comment:16 Changed 6 months ago by teor

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Note: See TracTickets for help on using tickets.