Opened 12 months ago

Closed 10 months ago

Last modified 9 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 12 months ago by nickm

I think #19999 was the ticket

comment:2 Changed 12 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 12 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 12 months ago by dgoulet (previous) (diff)

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

Owner: set to nickm
Status: newaccepted

comment:6 Changed 11 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 11 months ago by dgoulet

Reviewer: juga

comment:8 in reply to:  6 Changed 11 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 11 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 11 months ago by nickm

Status: needs_revisionneeds_review

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

comment:11 Changed 10 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 10 months ago by asn

Reviewer: jugajuga,asn

comment:13 Changed 10 months ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:14 Changed 10 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:15 Changed 10 months ago by teor

Keywords: 035-backport added

We backported this change to 0.3.5.8, but no further.

comment:16 Changed 9 months ago by teor

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