Opened 3 years ago

Closed 3 years ago

#19999 closed defect (implemented)

Maybe test-cases should complete without BUG warnings?

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: testing, TorCoreTeam201609, review-group-10
Cc: Actual Points: 3
Parent ID: Points: 2
Reviewer: Sponsor:

Description

If you run the unit tests with the --warn option, you fill find a lot of warnings. We deliberately test warning-paths, so that's not too bad...

... but you'll also see a lot of "Bug" warnings, and that's not so great. Some of them even cause stack traces.

I think we should go over these before 0.2.9 finishes, too make sure they aren't causing actual bugs.

Child Tickets

TicketStatusOwnerSummaryComponent
#20042closednickmAlways log BUG messages from the unit testsCore Tor/Tor

Change History (16)

comment:1 Changed 3 years ago by nickm

We could at least enforce that no BUG() or tor_assert_nonfatal() violations happen during unit tests.

comment:2 Changed 3 years ago by nickm

My #20042 branch makes it harder to accidentally suppress log messages, and makes BUG messages on-by-default. While doing that, I found #20041, where running the tests at --debug made one fail. (!)

comment:3 Changed 3 years ago by nickm

Now we've got a huge pile of warnings to fix. By my count, roughly 400 lines with "Bug: " in them.

comment:4 Changed 3 years ago by nickm

Down to 17.

Also, as of 3269307dafafa8c7c2c3e0be5d5c3cb5e7df3153 , it counts as a test failure any time that tor_assert_nonfatal() fails.

comment:5 Changed 3 years ago by nickm

Actual Points: 3
Keywords: TorCoreTeam201609 added
Owner: set to nickm
Status: newaccepted

Down to 0

comment:6 Changed 3 years ago by nickm

Resolution: fixed
Status: acceptedclosed

comment:7 Changed 3 years ago by rubiate

If util/time fails it doesn't call teardown_capture_of_logs()

diff --git a/src/test/test_util.c b/src/test/test_util.c
index 5949eb9..224ec7b 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -262,7 +262,6 @@ test_util_time(void *arg)
   time_t t_res;
   int i;
   struct timeval tv;
-  int old_log_level = 0;
 
   /* Test tv_udiff and tv_mdiff */
 
@@ -1112,8 +1111,7 @@ test_util_time(void *arg)
 #undef CHECK_TIMEGM_ARG_OUT_OF_RANGE
 
  done:
-  if (old_log_level)
-    teardown_capture_of_logs();
+  teardown_capture_of_logs();
 }
 
 static void

Similarly for tortls:

diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c
index 790c331..8502e8a 100644
--- a/src/test/test_tortls.c
+++ b/src/test/test_tortls.c
@@ -2278,7 +2278,6 @@ test_tortls_finish_handshake(void *ignored)
 
   X509 *c1 = read_cert_from(validCertString);
   SESS_CERT_local *sess = NULL;
-  int log_level = 0;
 
   ctx = SSL_CTX_new(method);
 
@@ -2298,7 +2297,6 @@ test_tortls_finish_handshake(void *ignored)
   expect_single_log_msg_containing("For some reason, wasV2Handshake didn't "
                                    "get set.");
   teardown_capture_of_logs();
-  log_level = 0;
 
   tls->wasV2Handshake = 1;
   ret = tor_tls_finish_handshake(tls);
@@ -2337,8 +2335,7 @@ test_tortls_finish_handshake(void *ignored)
   tor_free(tls);
   SSL_CTX_free(ctx);
   tor_free(method);
-  if (log_level)
-    teardown_capture_of_logs();
+  teardown_capture_of_logs();
 }
 #endif

comment:8 Changed 3 years ago by nickm

Thanks! I thought I'd caught all of those...

comment:9 Changed 3 years ago by nickm

Applied as 63e34e9e49e8514e2edfdc8e964bfc5752ca6326

comment:10 Changed 3 years ago by rubiate

rend_cache/store_v2_desc_as_client will occasionally cough up a warning:

[warn] rend_cache_store_v2_desc_as_client(): Bug: Couldn't decode base32 [scrubbed] for descriptor id. (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)

The problem is on test_rendcache.c:158, it tries to test an incorrect ID by incrementing the first character of the base32 ID, which can make it invalid base32 depending on what the first character is. The test still passes because rend_cache_store_v2_desc_as_client() still fails, just for a different reason.

Possible fix, go down if it can't go up:

diff --git a/src/test/test_rendcache.c b/src/test/test_rendcache.c
index a5d3f35..baadf44 100644
--- a/src/test/test_rendcache.c
+++ b/src/test/test_rendcache.c
@@ -87,7 +87,7 @@ test_rend_cache_lookup_entry(void *data)
 static void
 test_rend_cache_store_v2_desc_as_client(void *data)
 {
-  int ret;
+  int ret, mod;
   rend_data_t *mock_rend_query;
   char desc_id_base32[REND_DESC_ID_V2_LEN_BASE32 + 1];
   rend_cache_entry_t *entry = NULL;
@@ -155,12 +155,19 @@ test_rend_cache_store_v2_desc_as_client(void *data)
   // Test incorrect descriptor ID
   rend_cache_init();
   mock_rend_query = mock_rend_data(service_id);
-  desc_id_base32[0]++;
+
+  /* avoid incrementing beyond valid base32 characters */
+  mod = (desc_id_base32[0] == 'z' ||
+         desc_id_base32[0] == 'Z' ||
+         desc_id_base32[0] == '7') ? -1 : 1;
+
+  desc_id_base32[0] += mod;
+
   ret = rend_cache_store_v2_desc_as_client(desc_holder->desc_str,
                                            desc_id_base32, mock_rend_query,
                                            NULL);
   tt_int_op(ret, OP_EQ, -1);
-  desc_id_base32[0]--;
+  desc_id_base32[0] -= mod;
   rend_cache_free_all();

   // Test too old descriptor

comment:11 Changed 3 years ago by nickm

Resolution: fixed
Status: closedreopened

comment:12 Changed 3 years ago by nickm

Status: reopenedneeds_review

comment:13 Changed 3 years ago by rubiate

Tangentially related; addr/ip6_helpers will usually crash if run with --verbose

The loop which builds the string in src/test/test_addr.c:88 never gets run because it declares ii_=0 but uses i as the condition, and i is already incremented to 16 on test_addr.c:278

Possible fix: (using j here instead of ii_ to avoid touching every line realigning the backslashes)

diff --git a/src/test/test_addr.c b/src/test/test_addr.c
index c8a9e6d..1d91194 100644
--- a/src/test/test_addr.c
+++ b/src/test/test_addr.c
@@ -85,10 +85,10 @@ test_addr_basic(void *arg)
     char *, "%s",                                                \
     { char *cp;                                                  \
       cp = print_ = tor_malloc(64);                              \
-      for (int ii_=0;i<16;++i) {                                 \
-        tor_snprintf(cp, 3,"%02x", (unsigned)value_->s6_addr[i]);\
+      for (int j=0;j<16;++j) {                                   \
+        tor_snprintf(cp, 3,"%02x", (unsigned)value_->s6_addr[j]);\
         cp += 2;                                                 \
-        if (ii_ != 15) *cp++ = ':';                              \
+        if (j != 15) *cp++ = ':';                                \
       }                                                          \
     },                                                           \
     { tor_free(print_); },                                       \

comment:14 Changed 3 years ago by nickm

Keywords: review-group-10 added

Add needs_review 0.2.9 tickets, plus ones that have been in needs_revision for less than a week, to review-group-10.

comment:15 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Status: needs_reviewnew

rubiate: merged your patch as baa6be9fe7fb 0e1b228aa6b5ed766. Thanks!

Last edited 3 years ago by nickm (previous) (diff)

comment:16 Changed 3 years ago by nickm

Resolution: implemented
Status: newclosed
Note: See TracTickets for help on using tickets.