Skip to content

Consolidate redundant unit and e2e tests (cbor, http, logger, receipts)#8026

Draft
achamayou wants to merge 10 commits into
microsoft:mainfrom
achamayou:copilot/consolidate-redundant-tests
Draft

Consolidate redundant unit and e2e tests (cbor, http, logger, receipts)#8026
achamayou wants to merge 10 commits into
microsoft:mainfrom
achamayou:copilot/consolidate-redundant-tests

Conversation

@achamayou

@achamayou achamayou commented Jul 4, 2026

Copy link
Copy Markdown
Member

Summary

Consolidates several redundant/duplicated tests across the C++ unit test suite and the Python e2e suite, keeping the same behavioural coverage in fewer, clearer test cases.

Changes

  • src/crypto/test/cbor.cpp: merges the two separate make_signed positive/negative test cases into a single parameterized test, using doctest's DOCTEST_VALUE_PARAMETERIZED_DATA pattern.
  • src/http/test/http_test.cpp: consolidates the parse_want_repr_digest test cases into a single test with subcases, removing duplicated setup.
  • src/ds/test/logger.cpp / src/ds/test/logger_json_test.cpp: moves the standalone JSON log format test into the existing logger unit test file, removing the now-redundant logger_json_test binary/target. The moved test now uses RAII guards to reliably restore global logger configuration (loggers, log level) and std::cout's redirected buffer, even if a REQUIRE fails or an exception is thrown.
  • CMakeLists.txt: removes the logger_json_test unit-test target (superseded by the above).
  • tests/e2e_logging.py: folds the deterministic "fresh tx" receipt checks from test_receipts into test_random_receipts via a new issue_txs_for_receipt_check helper, which issues a batch of transactions and waits for the last one to commit (via infra.commit.wait_for_commit) before returning their seqnos - avoiding flaky NOT_FOUND historical receipt lookups for not-yet-committed COSE receipts.

Testing

  • cbor_test: 61/61 test cases, 553/553 assertions passing.
  • logger_test: 4/4 test cases, 61/61 assertions passing.
  • tests/e2e_logging.py: syntax/lint-checked (py_compile, ruff); full e2e coverage exercised via existing CI.
  • scripts/ci-checks.sh -f (C++/Python formatting, ASCII, copyright) run clean.

Copilot AI and others added 4 commits July 3, 2026 21:14
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
…fact

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 4, 2026 13:27
@achamayou achamayou requested a review from a team as a code owner July 4, 2026 13:27
Comment thread .gitignore Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates several redundant tests across the Python e2e suite and C++ unit tests, aiming to reduce duplicated coverage while keeping the same behavioral checks in fewer places.

Changes:

  • Refactors tests/e2e_logging.py receipt testing by folding deterministic “fresh tx” coverage into test_random_receipts via a helper that returns additional seqnos to validate.
  • Consolidates/streamlines multiple C++ unit tests using DOCTEST_SUBCASE/SUBCASE patterns (HTTP header parsing and CBOR helpers).
  • Moves the JSON logger format test into the existing logger unit test file and removes the standalone logger_json_test target.

Custom instructions used:

  • .github/copilot-instructions.md
  • .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/e2e_logging.py Reworks receipt testing to reuse test_random_receipts with an additional batch of freshly-issued tx seqnos.
src/http/test/http_test.cpp Consolidates parse_want_repr_digest coverage into a single test case with subcases.
src/ds/test/logger.cpp Adds JSON log format validation into existing logger unit tests.
src/ds/test/logger_json_test.cpp Removes the now-redundant standalone JSON logger unit test.
src/crypto/test/cbor.cpp Merges duplicated make_signed tests into one test case with subcases.
CMakeLists.txt Removes the logger_json_test unit-test target.
.gitignore Adds ignore entry for a logger test output file.

Comment thread tests/e2e_logging.py
Comment thread src/ds/test/logger.cpp Outdated
Comment thread src/ds/test/logger.cpp Outdated
@achamayou achamayou marked this pull request as draft July 4, 2026 13:34
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread src/crypto/test/cbor.cpp Outdated
GitHub Copilot and others added 2 commits July 6, 2026 17:24
- src/crypto/test/cbor.cpp: parameterize the make_signed test using
  doctest's DOCTEST_VALUE_PARAMETERIZED_DATA pattern instead of manual
  SUBCASEs.
- src/ds/test/logger.cpp: restore global logger config (loggers and
  level) at the end of the custom log format test via an RAII guard,
  to avoid leaking state into other test cases.
- tests/e2e_logging.py: wait for the last issued transaction to commit
  in issue_txs_for_receipt_check before returning, to avoid flaky
  NOT_FOUND historical receipt lookups for COSE-only builds.
@achamayou achamayou changed the title Copilot/consolidate redundant tests Consolidate redundant unit and e2e tests (cbor, http, logger, receipts) Jul 6, 2026
@achamayou

Copy link
Copy Markdown
Member Author

Paused on doctest update

GitHub Copilot and others added 2 commits July 9, 2026 10:46
- e2e_logging: assert receipts for known-committed seqnos are actually verified (require_additional_receipts) instead of silently skipping on poll timeout
- logger test: use PID-based temp file and clear the logger set for isolation from other test cases in the shared binary
- cbor test: clarify make_signed test variable names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants