From 5d5ffccbe4aa1072246456019c04c7b372ddccb7 Mon Sep 17 00:00:00 2001 From: LTLA Date: Wed, 8 Jan 2025 16:05:05 -0800 Subject: [PATCH] More comprehensive tests against any assumption of zeroed arrays. We use a test-only macro to insert a non-zero initial value that changes with each invocation. So, any reliance on a zeroed input array would manifest as different results across repeated calls in the test suite. In effect, all tests are now checking against "dirty" input buffers, eliminating the need for dedicated tests with less coverage. --- .github/workflows/run-tests.yaml | 12 ++- .../aggregate_across_cells.hpp | 12 ++- .../aggregate_across_genes.hpp | 6 +- tests/CMakeLists.txt | 6 ++ tests/src/aggregate_across_cells.cpp | 54 +----------- tests/src/aggregate_across_genes.cpp | 82 ++++--------------- tests/src/clean_factor.cpp | 8 +- tests/src/combine_factors.cpp | 13 ++- tests/src/utils.h | 19 +++++ 9 files changed, 84 insertions(+), 128 deletions(-) create mode 100644 tests/src/utils.h diff --git a/.github/workflows/run-tests.yaml b/.github/workflows/run-tests.yaml index 433a578..e0e8601 100644 --- a/.github/workflows/run-tests.yaml +++ b/.github/workflows/run-tests.yaml @@ -19,10 +19,14 @@ jobs: os: ubuntu-latest, cov: true } + - { + name: "Ubuntu Latest GCC, dirty initialization", + os: ubuntu-latest, + dirty: true + } - { name: "macOS Latest Clang", - os: macos-latest, - cov: false + os: macos-latest } steps: @@ -35,6 +39,10 @@ jobs: if: ${{ matrix.config.cov }} run: cmake -S . -B build -DCODE_COVERAGE=ON + - name: Configure the build with coverage + if: ${{ matrix.config.dirty }} + run: cmake -S . -B build -DTEST_DIRTY_INIT=ON + - name: Configure the build with custom parallelization if: ${{ ! matrix.config.cov }} run: cmake -S . -B build diff --git a/include/scran_aggregate/aggregate_across_cells.hpp b/include/scran_aggregate/aggregate_across_cells.hpp index 99ac4d6..391d597 100644 --- a/include/scran_aggregate/aggregate_across_cells.hpp +++ b/include/scran_aggregate/aggregate_across_cells.hpp @@ -317,7 +317,11 @@ AggregateAcrossCellsResults aggregate_across_cells( AggregateAcrossCellsBuffers buffers; if (options.compute_sums) { - output.sums.resize(nlevels, std::vector(ngenes)); + output.sums.resize(nlevels, std::vector(ngenes +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + )); buffers.sums.resize(nlevels); for (size_t l = 0; l < nlevels; ++l) { buffers.sums[l] = output.sums[l].data(); @@ -325,7 +329,11 @@ AggregateAcrossCellsResults aggregate_across_cells( } if (options.compute_detected) { - output.detected.resize(nlevels, std::vector(ngenes)); + output.detected.resize(nlevels, std::vector(ngenes +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + )); buffers.detected.resize(nlevels); for (size_t l = 0; l < nlevels; ++l) { buffers.detected[l] = output.detected[l].data(); diff --git a/include/scran_aggregate/aggregate_across_genes.hpp b/include/scran_aggregate/aggregate_across_genes.hpp index 3ad0c83..1ce52de 100644 --- a/include/scran_aggregate/aggregate_across_genes.hpp +++ b/include/scran_aggregate/aggregate_across_genes.hpp @@ -337,7 +337,11 @@ AggregateAcrossGenesResults aggregate_across_genes( buffers.sum.resize(nsets); for (size_t s = 0; s < nsets; ++s) { - output.sum[s].resize(NC); + output.sum[s].resize(NC +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + ); buffers.sum[s] = output.sum[s].data(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 694c7cf..f290368 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,5 +28,11 @@ if(CODE_COVERAGE AND CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") target_link_options(libtest PRIVATE --coverage) endif() +option(TEST_DIRTY_INIT "Test dirty vector initialization" OFF) +if(TEST_DIRTY_INIT) + target_compile_definitions(dirtytest PRIVATE "SCRAN_AGGREGATE_TEST_INIT=initial_value()") +endif() + include(GoogleTest) gtest_discover_tests(libtest) +gtest_discover_tests(dirtytest) diff --git a/tests/src/aggregate_across_cells.cpp b/tests/src/aggregate_across_cells.cpp index 9296eed..194477e 100644 --- a/tests/src/aggregate_across_cells.cpp +++ b/tests/src/aggregate_across_cells.cpp @@ -1,11 +1,11 @@ -#include - -#include "scran_aggregate/aggregate_across_cells.hpp" #include "scran_tests/scran_tests.hpp" #include #include +#include "utils.h" // must be included before scran_aggregate. +#include "scran_aggregate/aggregate_across_cells.hpp" + static std::vector create_groupings(size_t n, int ngroups) { std::vector groupings(n); for (size_t g = 0; g < groupings.size(); ++g) { @@ -116,51 +116,3 @@ TEST(AggregateAcrossCells, Skipping) { EXPECT_EQ(skipped.sums.size(), 0); EXPECT_EQ(skipped.detected.size(), 0); } - -TEST(AggregateAcrossCells, DirtyBuffers) { - int nr = 88, nc = 126; - auto vec = scran_tests::simulate_vector(nr * nc, []{ - scran_tests::SimulationParameters sparams; - sparams.density = 0.1; - sparams.seed = 69; - return sparams; - }()); - - tatami::DenseRowMatrix dense_row(nr, nc, std::move(vec)); - auto sparse_column = tatami::convert_to_compressed_sparse(&dense_row, false); - size_t ngroups = 3; - auto grouping = create_groupings(dense_row.ncol(), ngroups); - - // Setting up some dirty buffers. - scran_aggregate::AggregateAcrossCellsResults store; - scran_aggregate::AggregateAcrossCellsBuffers buffers; - store.sums.resize(ngroups, std::vector(nr, -1)); - store.detected.resize(ngroups, std::vector(nr, -1)); - buffers.sums.resize(ngroups); - buffers.detected.resize(ngroups); - for (size_t l = 0; l < ngroups; ++l) { - buffers.sums[l] = store.sums[l].data(); - buffers.detected[l] = store.detected[l].data(); - } - - scran_aggregate::AggregateAcrossCellsOptions opt; - auto ref = scran_aggregate::aggregate_across_cells(dense_row, grouping.data(), opt); - scran_aggregate::aggregate_across_cells(dense_row, grouping.data(), buffers, opt); - EXPECT_EQ(ref.sums.size(), store.sums.size()); - EXPECT_EQ(ref.detected.size(), store.detected.size()); - for (size_t l = 0; l < ngroups; ++l) { - EXPECT_EQ(ref.sums[l], store.sums[l]); - EXPECT_EQ(ref.detected[l], store.detected[l]); - } - - // Same for column-major iteration. - for (size_t l = 0; l < ngroups; ++l) { - std::fill_n(buffers.sums[l], nr, -1); - std::fill_n(buffers.detected[l], nr, -1); - } - scran_aggregate::aggregate_across_cells(*sparse_column, grouping.data(), buffers, opt); - for (size_t l = 0; l < ngroups; ++l) { - EXPECT_EQ(ref.sums[l], store.sums[l]); - EXPECT_EQ(ref.detected[l], store.detected[l]); - } -} diff --git a/tests/src/aggregate_across_genes.cpp b/tests/src/aggregate_across_genes.cpp index 47dc79d..c75b454 100644 --- a/tests/src/aggregate_across_genes.cpp +++ b/tests/src/aggregate_across_genes.cpp @@ -1,25 +1,10 @@ -#include - -#include "scran_aggregate/aggregate_across_genes.hpp" #include "scran_tests/scran_tests.hpp" + #include #include -static std::vector > create_sets(size_t nsets, int ngenes, size_t seed) { - std::vector > groupings(nsets); - std::mt19937_64 rng(seed); - std::uniform_real_distribution runif; - for (auto& grp : groupings) { - for (int g = 0; g < ngenes; ++g) { - if (runif(rng) < 0.15) { - grp.push_back(g); - } - } - } - return groupings; -} - -/*********************************************/ +#include "utils.h" // must be included before scran_aggregate +#include "scran_aggregate/aggregate_across_genes.hpp" class AggregateAcrossGenesTest : public ::testing::TestWithParam { protected: @@ -45,10 +30,19 @@ TEST_P(AggregateAcrossGenesTest, Unweighted) { const size_t nsets = 100; int ngenes = dense_row->nrow(); - auto mock_sets = create_sets(nsets, ngenes, /* seed = */ nsets * nthreads); + std::vector > mock_sets(nsets); + std::mt19937_64 rng(nsets * nthreads); + std::uniform_real_distribution runif; + for (auto& grp : mock_sets) { + for (int g = 0; g < ngenes; ++g) { + if (runif(rng) < 0.15) { + grp.push_back(g); + } + } + } std::vector > gene_sets; - gene_sets.reserve(mock_sets.size()); + gene_sets.reserve(nsets); for (const auto& grp : mock_sets) { gene_sets.emplace_back(grp.size(), grp.data(), static_cast(NULL)); } @@ -204,51 +198,3 @@ TEST(AggregateAcrossGenes, OutOfRange) { scran_aggregate::aggregate_across_genes(mat, gene_sets, opt); }, "out of range"); } - -TEST(AggregateAcrossGenes, DirtyBuffers) { - int nr = 110, nc = 78; - auto vec = scran_tests::simulate_vector(nr * nc, []{ - scran_tests::SimulationParameters sparams; - sparams.density = 0.1; - return sparams; - }()); - - tatami::DenseRowMatrix dense_row(nr, nc, std::move(vec)); - auto sparse_column = tatami::convert_to_compressed_sparse(&dense_row, false); - - // Setting up the gene sets. - size_t nsets = 100; - auto mock_sets = create_sets(nsets, nr, /* seed = */ 99); - - std::vector > gene_sets; - gene_sets.reserve(mock_sets.size()); - for (const auto& grp : mock_sets) { - gene_sets.emplace_back(grp.size(), grp.data(), static_cast(NULL)); - } - - // Setting up some buffers. - scran_aggregate::AggregateAcrossGenesResults store; - scran_aggregate::AggregateAcrossGenesBuffers buffers; - store.sum.resize(nsets); - buffers.sum.resize(nsets); - for (size_t s = 0; s < nsets; ++s) { - store.sum[s].resize(nc, -1); // using -1 to simulate uninitialized values. - buffers.sum[s] = store.sum[s].data(); - } - - // Checking that the dirtiness doesn't affect the results. - scran_aggregate::AggregateAcrossGenesOptions opt; - auto ref = scran_aggregate::aggregate_across_genes(dense_row, gene_sets, opt); - scran_aggregate::aggregate_across_genes(dense_row, gene_sets, buffers, opt); - for (size_t s = 0; s < nsets; ++s) { - EXPECT_EQ(ref.sum[s], store.sum[s]); - } - - for (size_t s = 0; s < nsets; ++s) { - std::fill_n(store.sum[s].data(), nc, -1); // dirtying it again for another run. - } - scran_aggregate::aggregate_across_genes(*sparse_column, gene_sets, buffers, opt); - for (size_t s = 0; s < nsets; ++s) { - EXPECT_EQ(ref.sum[s], store.sum[s]); - } -} diff --git a/tests/src/clean_factor.cpp b/tests/src/clean_factor.cpp index a528725..8c084df 100644 --- a/tests/src/clean_factor.cpp +++ b/tests/src/clean_factor.cpp @@ -2,12 +2,16 @@ #include +#include "utils.h" // must be included before scran_aggregate #include "scran_aggregate/clean_factor.hpp" -#include "scran_aggregate/combine_factors.hpp" template std::pair, std::vector > test_clean_factor(size_t n, const Factor_* factor) { - std::vector cleand(n); + std::vector cleand(n +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + ); auto levels = scran_aggregate::clean_factor(n, factor, cleand.data()); return std::make_pair(std::move(levels), std::move(cleand)); } diff --git a/tests/src/combine_factors.cpp b/tests/src/combine_factors.cpp index a62511f..31388c5 100644 --- a/tests/src/combine_factors.cpp +++ b/tests/src/combine_factors.cpp @@ -2,11 +2,16 @@ #include +#include "utils.h" // must be included before scran_aggregate #include "scran_aggregate/combine_factors.hpp" template std::pair >, std::vector > test_combine_factors(size_t n, const std::vector& factors) { - std::vector combined(n); + std::vector combined(n +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + ); auto levels = scran_aggregate::combine_factors(n, factors, combined.data()); return std::make_pair(std::move(levels), std::move(combined)); } @@ -131,7 +136,11 @@ TEST(CombineFactors, Multiple) { template std::pair >, std::vector > test_combine_factors_unused(size_t n, const std::vector >& factors) { - std::vector combined(n); + std::vector combined(n +#ifdef SCRAN_AGGREGATE_TEST_INIT + , SCRAN_AGGREGATE_TEST_INIT +#endif + ); auto levels = scran_aggregate::combine_factors_unused(n, factors, combined.data()); return std::make_pair(std::move(levels), std::move(combined)); } diff --git a/tests/src/utils.h b/tests/src/utils.h new file mode 100644 index 0000000..732a570 --- /dev/null +++ b/tests/src/utils.h @@ -0,0 +1,19 @@ +#ifndef UTILS_H +#define UTILS_H + +// This generates a different initial value for each vector() that might store results. +// The aim is to check that our functions ignore the initial contents of each output array. +// Otherwise, we might miss subtle bugs from an incorrect assumption of default-initialized contents. +// We make the values vary across calls rather than using a constant, +// as this allows us to catch bugs via tests that check for consistency between calls. +inline int initial_value() { + static int counter = 0; + if (counter == 255) { + counter = 1; + } else { + ++counter; + } + return counter; +} + +#endif