(#513) Refactor into set_log_streams_for_tests() to manage unit-test outputs.#514
(#513) Refactor into set_log_streams_for_tests() to manage unit-test outputs.#514
Conversation
✅ Deploy Preview for splinterdb canceled.
|
| # defined above using unit_test_self_dependency. | ||
| # | ||
| $(BINDIR)/$(UNITDIR)/misc_test: $(UTIL_SYS) | ||
| $(BINDIR)/$(UNITDIR)/misc_test: $(UTIL_SYS) $(COMMON_UNIT_TESTOBJ) |
There was a problem hiding this comment.
I thought about including this new .o in COMMON_TESTOBJ symbol, but that would mean we are mixing up shared .o's from diff sub-dirs.
The way it's done is a bit more work to keep adding this COMMON_UNIT_TESTOBJ dependency in several lines, but it's at least clear, and cleaner, to specify exactly which unit_test-dot-oh depends on which common object.
| if (Ctest_verbose) { | ||
| platform_set_log_streams(stdout, stderr); | ||
| } | ||
| set_log_streams_for_tests(); |
There was a problem hiding this comment.
For symmetry, I introduced this set_ method, too. This is really not needed, but it makes it easier for new unit-test dev to just follow one set_log_streams_for*() pattern.
|
|
||
| /* Name of SplinterDB device created for unit-tests */ | ||
| #define TEST_DB_NAME "unit_tests_db" | ||
| #define TEST_DB_NAME "splinterdb_unit_tests_db" |
There was a problem hiding this comment.
Slightly unrelated bug-fix ... arising from one of @rosenhouse 's suggestion on an earlier PR.
gapisback
force-pushed
the
agurajada/513-ctest-output
branch
from
03a7fb2 to
e2c9a83
Compare
gapisback
force-pushed
the
agurajada/513-ctest-output
branch
from
e2c9a83 to
67335d2
Compare
| # unit-tests (and won't pick-up test-specific common files, e.g. | ||
| # btree_test_common.c) | ||
| COMMON_UNIT_TESTSRC := $(shell find $(UNIT_TESTSDIR) -name "*tests_common.c") | ||
| COMMON_UNIT_TESTSRC := $(wildcard $(UNIT_TESTSDIR)/*tests_common.c) |
There was a problem hiding this comment.
Comes about due to changes introduced under earlier PR integrated at SHA 615050a to remove use of shell built-ins in favor of using Makefile built-in commands.
gapisback
force-pushed
the
agurajada/513-ctest-output
branch
from
67335d2 to
40a805f
Compare
gapisback
force-pushed
the
agurajada/513-ctest-output
branch
from
40a805f to
003fe51
Compare
|
Small reminder @rosenhouse - to see if you can spend some time on this review. I just rebased off of /main and pushed an updated commit set. Thanks! |
rosenhouse
left a comment
rosenhouse
left a comment
There was a problem hiding this comment.
Definitely an improvement, though I would prefer a different name for the error-case function.
This commit refactors existing chunks of code that exists in different unit- test sources to manage output file handles to a common function. This is defined in new file unit/unit_tests_common.c ; set_log_streams_for_tests() Tests that check error-raising behaviour will now need to call set_log_streams_for_error_tests(), to manage output streams. Minor correction to TEST_DB_NAME; change it to conform to the r.e. defined in .gitignore to suppress listing this in 'git status' output.
gapisback
force-pushed
the
agurajada/513-ctest-output
branch
from
003fe51 to
5d05ab9
Compare
3 participants
This commit refactors existing chunks of code that exists in different unit- test sources to manage output file handles to a common function, now defined in new file
unit/unit_tests_common.c;set_log_streams_for_tests().Tests that check error-raising behaviour will now need to call
set_log_streams_for_neg_tests(), to manage output streams.Minor correction to
TEST_DB_NAME; change it to conform to the r.e. defined in .gitignore to suppress listing this in 'git status' output.NOTE: I did not try to make this even more generic beyond unit-tests to have
functional/ testsalso call into these interfaces. That's why this new fileunit_tests_common.cis intests/unitsub-dir and not in top-leveltests/dir.I think, let's see how this settles down and if we ever find the need to make functional tests also get this capability, we can consider relocating this new file to
tests/sub-dir.