Opened 8 years ago

Closed 8 years ago

#1895 closed defect (fixed)

regression fix: set B10_FROM_BUILD for auth config tests

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120501
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

We need to specify B10_FROM_BUILD in
MemoryDatasrcConfigTest.addOneWithFiletypeSQLite3
for the loadable data source module. This will fix regressions like
this:
http://git.bind10.isc.org/~tester/builder//BIND10/20120416211101-OpenBSD5-amd64/logs/unittests.out

This is not a one-liner fix, so I'm creating a separate ticket.
I'll push it to the current sprint for its urgency.

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jinmei

  • Status changed from new to reviewing

The proposed fix is in the trac1895 branch. Could someone review it
ASAP?

Thanks,

comment:2 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:3 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Why not set environment variable for tests directly, like we do for a number of other tests?

e.g. put run_unittests into check-local instead of TESTS and set B10_FROM_BUILD there

diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am
index b33c7af..682c88e 100644
--- a/src/bin/auth/tests/Makefile.am
+++ b/src/bin/auth/tests/Makefile.am
@@ -19,10 +19,11 @@ endif
 
 CLEANFILES = *.gcno *.gcda
 
+# Do not define global tests, use check-local so
+# environment can be set (needed for dynamic loading)
 TESTS =
 if HAVE_GTEST
 
-TESTS += run_unittests
 run_unittests_SOURCES = $(top_srcdir)/src/lib/dns/tests/unittest_util.h
 run_unittests_SOURCES += $(top_srcdir)/src/lib/dns/tests/unittest_util.cc
 run_unittests_SOURCES += ../auth_srv.h ../auth_srv.cc
@@ -70,6 +71,8 @@ run_unittests_LDADD += $(top_builddir)/src/lib/server_common/l
 run_unittests_LDADD += $(top_builddir)/src/lib/nsas/libnsas.la
 run_unittests_LDADD += $(top_builddir)/src/lib/util/unittests/libutil_unittests
 run_unittests_LDADD += $(top_builddir)/src/lib/statistics/libstatistics.la
+
+check-local:
+       B10_FROM_BUILD=${abs_top_builddir} ./run_unittests
 endif
 
-noinst_PROGRAMS = $(TESTS)
+noinst_PROGRAMS = run_unittests

If there is a legit reason to do this from within the test code itself, a few small comments:

  • since followup may be borked, EXPECT_EQ should probably be ASSERT_EQ in TearDown? (though it won't matter in practice, since it is already done, it would be slightly more indicative of the seriousness :)

"Couldn't restore environment, results of other tests" and "are uncertain"; need a space at the end of the first or the start of the second string

Last edited 8 years ago by jelte (previous) (diff)

comment:4 follow-up: Changed 8 years ago by jelte

after a short talk with jeremy on jabber I pushed the diff proposed in the previous comment to master, to get the tests running again, but leaving the ticket open for further discussion (and/or reverting it and applying the branch itself, depending on the outcome of said discussion)

comment:5 in reply to: ↑ 3 Changed 8 years ago by jinmei

Replying to jelte:

Why not set environment variable for tests directly, like we do for a number of other tests?

Because I wanted to make it workable even if we invoke the executable
directly, maybe for specifying gtest filter. We could introduce a
separate wrapper shell script, but it seemed to be more error prone
(we can easily overlook the need for the script).

If there is a legit reason to do this from within the test code itself, a few small comments:

After reading the comment, I updated my mind a bit and switched to
setting this variable globally for the entire tests. At least it
doesn't break any other tests right now, and I guess we'll also need
it for other cases pretty soon (at least for supporting the reload
command for inmem-over-sqlite3, and eventually other general cases
when we enable the factory).

I've clarified these points as comments in main().

So the following points may become moot now, but responding to them
anyway...

  • since followup may be borked, EXPECT_EQ should probably be ASSERT_EQ in TearDown? (though it won't matter in practice, since it is already done, it would be slightly more indicative of the seriousness :)

Hmm, is ASSERT_EQ better in this case? So we don't try to restore the
rest of the variables? I do not necessarily object to that, but am
not sure if that's definitely better than EXPECT_EQ (in which case we
leave a warning for the failure case but still try to do the best
restoring as many as possible). Since failure of this is something
very unexpected, if we worry about the seriousness I'd rather assert()
that.

"Couldn't restore environment, results of other tests" and "are uncertain"; need a space at the end of the first or the start of the second string

Ah, right. It was actually an issue of the code in common_unittest
that I copied for this branch. I've fixed the original part.

comment:6 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:7 in reply to: ↑ 4 Changed 8 years ago by jinmei

  • Priority changed from very high to medium

Replying to jelte:

after a short talk with jeremy on jabber I pushed the diff proposed in the previous comment to master, to get the tests running again, but leaving the ticket open for further discussion (and/or reverting it and applying the branch itself, depending on the outcome of said discussion)

Ah, okay, so the priority of this task doesn't have to be 'very high'
any more. I'll reduce it.

comment:8 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Ok, I've looked into it briefly and changing the API of the factory to optionally provide a path is easy, but using it correctly is slightly harder; we can hardcode the path based on TOP_BUILD_DIR quite easily in most tests, but due to the way the memory-ds-config-handler initializes the sqlite3 one, it would be slightly nontrivial there (around line 180 of auth_config.cc).

I don't personally mind having to call make check, so I'm fine with what I proposed (or I wouldn't have proposed it), but I'm also not opposed to reverting that and putting in this branch. And make a ticket to change that API and do it right :) (probably after or during the datasource config changes which are currently being discussed).

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to jelte:

Ok, I've looked into it briefly and changing the API of the factory to optionally provide a path is easy, but using it correctly is slightly harder; we can hardcode the path based on TOP_BUILD_DIR quite easily in most tests, but due to the way the memory-ds-config-handler initializes the sqlite3 one, it would be slightly nontrivial there (around line 180 of auth_config.cc).

I don't personally mind having to call make check, so I'm fine with what I proposed (or I wouldn't have proposed it), but I'm also not opposed to reverting that and putting in this branch. And make a ticket to change that API and do it right :) (probably after or during the datasource config changes which are currently being discussed).

For the auth unit tests I don't mind always doing 'make check' because
it's pretty fast, at least for now. Hopefully we can make it cleaner
when we cleanup data source configurations, and so I'll drop this
branch only with cherry-picking the editorial fix to the unrelated
part.

I'll then close this ticket.

comment:10 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 0 to 2
  • Resolution set to fixed
  • Status changed from reviewing to closed

closing, and giving an estimation point of 2.

Note: See TracTickets for help on using tickets.