Skip to content

Add direct coverage for serialization helpers and tree boundaries#32

Merged
achamayou merged 8 commits into
mainfrom
copilot/add-test-coverage-helpers
Jun 5, 2026
Merged

Add direct coverage for serialization helpers and tree boundaries#32
achamayou merged 8 commits into
mainfrom
copilot/add-test-coverage-helpers

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 19, 2026 •
edited
Loading

  • Inspect CI workflow runs and collect compiler error logs
  • Reproduce the compiler error locally
  • Apply minimal code fix for the reported compiler issue
  • Run targeted tests/build to verify the fix
  • Run final validation (code review + CodeQL) and report back

Copilot AI and others added 6 commits May 19, 2026 19:09
@achamayou
Add coverage tests for serialization and tree semantics
60f1513 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Fix coverage test build issues
e36939b 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Format coverage test
97c78f0 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Deduplicate partial range validation
842776c 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Clarify partial range validation
7744b93 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Document self-assignment coverage
df1e73a 
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/cd6a239e-ca77-480e-b515-6c171a869977

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copilot AI requested a review from achamayou May 19, 2026 19:19
@achamayou achamayou marked this pull request as ready for review May 19, 2026 19:46
Copilot AI review requested due to automatic review settings May 19, 2026 19:46
Copilot AI reviewed May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 adds a dedicated “coverage” test binary to exercise low-level serialization helpers and boundary behaviors, while tightening PathT equality semantics and centralizing validation for partial tree serialization ranges.

Changes:

  • Add test/coverage.cpp to directly cover serialization helpers, PathT metadata/deserialization/equality, partial-serialization boundary cases, and TreeT move/self-assignment.
  • Update PathT equality to include leaf/max-index metadata and make operator!= const.
  • Centralize partial range validation for TreeT::serialise(from,to) and TreeT::serialised_size(from,to), including explicit empty-tree rejection.

Reviewed changes

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

File Description
test/coverage.cpp New targeted coverage test executable covering helper functions and boundary/error paths.
test/CMakeLists.txt Registers the new coverage test target with the existing test harness.
merklecpp.h Refines PathT equality/inequality semantics and factors partial-range validation into a shared helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@achamayou
Copy link
Copy Markdown
Member

achamayou commented May 20, 2026

@copilot look at the compiler errors and fix them

Copilot AI and others added 2 commits May 20, 2026 07:30
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 20, 2026

@copilot look at the compiler errors and fix them

Fixed. I investigated the CI logs, resolved the compiler/clippy-style errors in test/coverage.cpp, and fixed the crash by hardening TreeT move/copy assignment ownership handling. The fix is in commit 3f1edbb (plus earlier prep in 05797c0), and local + CI-like validation now pass.

@achamayou achamayou requested a review from Copilot June 2, 2026 21:07
Copilot AI reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

@achamayou achamayou requested a review from Copilot June 4, 2026 12:24
Copilot AI reviewed Jun 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

@achamayou achamayou merged commit 104a9de into main Jun 5, 2026
12 checks passed
maxtropets
maxtropets reviewed Jun 5, 2026
View reviewed changes
Comment thread test/coverage.cpp
}
catch (const std::exception& ex)
{
std::cout << "Error: " << ex.what() << '\n';
Copy link
Copy Markdown

@maxtropets maxtropets Jun 5, 2026

Choose a reason for hiding this comment

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

I think it's better to flush with std::endl to make sure we do not lose anything?

Copy link
Copy Markdown
Member

@achamayou achamayou Jun 5, 2026

Choose a reason for hiding this comment

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

I don't think we lose anything, the flushing may be a bit delayed, that's all. But this should arguably go to stderr, which is line buffered anyway.

maxtropets
maxtropets reviewed Jun 5, 2026
View reviewed changes
Comment thread merklecpp.h
const bool from_out_of_range = from < min_index() || max_index() < from;
const bool to_out_of_range = to < min_index() || max_index() < to;
const bool reversed_range = from > to;
if (empty() || from_out_of_range || to_out_of_range || reversed_range)
Copy link
Copy Markdown

@maxtropets maxtropets Jun 5, 2026

Choose a reason for hiding this comment

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

Nit. 3 comparisons are enough

min() <= from && from <= to && to <= max()

maxtropets
maxtropets reviewed Jun 5, 2026
View reviewed changes
Comment thread merklecpp.h
delete (n);
}
uninserted_leaf_nodes.clear();
insertion_stack.clear();
Copy link
Copy Markdown

@maxtropets maxtropets Jun 5, 2026

Choose a reason for hiding this comment

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

Just curious why .clear() and only then = move()?

Copy link
Copy Markdown

@maxtropets maxtropets Jun 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Also this's a dup of what we do in move ctor, so could've been made a single function, as in "clear()"

Copilot AI mentioned this pull request Jun 5, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@achamayou achamayou achamayou approved these changes

@maxtropets maxtropets maxtropets left review comments

Copilot code review Copilot Copilot left review comments

@cjen1-msft cjen1-msft cjen1-msft approved these changes

Assignees

@achamayou achamayou

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@achamayou @maxtropets @cjen1-msft