Opened 3 months ago

Closed 3 days 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.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, postfreeze-ok
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)
#29160defectclosednickmdo not LOG_ERR from test_address_get_if_addrs6 when there is no ipv6
#29161defectclosednickmExpect some gmtime/localtime() failures on windows, don't log bugs from tests.

Change History (14)

comment:1 Changed 3 months ago by nickm

I think #19999 was the ticket

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

comment:4 Changed 5 weeks 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 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:6 Changed 4 weeks 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 4 weeks ago by dgoulet

Reviewer: juga

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

Status: needs_revisionneeds_review

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

comment:11 Changed 11 days 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 4 days ago by asn

Reviewer: jugajuga,asn

comment:13 Changed 4 days ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:14 Changed 3 days ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.