Skip to content

Conversation

@nardoor
Copy link
Contributor

@nardoor nardoor commented Aug 31, 2025

No description provided.

@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from 9615f80 to 26d4e32 Compare September 15, 2025 21:08
@nardoor nardoor changed the title Draft: feat: broad snapshot id for all rustic commands feat(core): add get_snapshot_from_strs to Repository API Sep 15, 2025
@codecov
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.3%. Comparing base (59ad1df) to head (048d2af).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/backend/src/rclone.rs 0.0% 2 Missing ⚠️
crates/core/src/commands/snapshots.rs 87.5% 1 Missing ⚠️
crates/core/tests/integration.rs 66.6% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/core/src/commands/forget.rs 84.6% <ø> (ø)
crates/core/src/error.rs 67.1% <ø> (ø)
crates/core/src/repofile/configfile.rs 37.5% <ø> (ø)
crates/core/src/repository.rs 45.1% <100.0%> (+1.5%) ⬆️
crates/core/tests/integration/snapshots.rs 100.0% <100.0%> (ø)
crates/core/src/commands/snapshots.rs 80.0% <87.5%> (+55.0%) ⬆️
crates/core/tests/integration.rs 77.7% <66.6%> (-8.0%) ⬇️
crates/backend/src/rclone.rs 8.5% <0.0%> (-0.1%) ⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from 5bce55b to 6a3e8a1 Compare September 15, 2025 21:54
@nardoor nardoor changed the title feat(core): add get_snapshot_from_strs to Repository API feat(core): repository APIs to identify snapshots using latest~N Sep 15, 2025
@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from 6a3e8a1 to 7365fa5 Compare September 15, 2025 22:16
@nardoor nardoor requested a review from aawsome September 15, 2025 22:18
@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from be01e67 to 45e70c4 Compare September 21, 2025 17:33
@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from 31990fe to d2817ae Compare September 30, 2025 20:10
@aawsome
Copy link
Member

aawsome commented Oct 20, 2025

Hi @nardoor Is there something open from your side on this PR? IMO it should be only about some unit tests to increase test coverage..

@nardoor
Copy link
Contributor Author

nardoor commented Oct 20, 2025

Hi @nardoor Is there something open from your side on this PR? IMO it should be only about some unit tests to increase test coverage..

Hello @aawsome ,

You are totally right, I just need to add some tests. I'll get it done asap.
Sorry for the delay

@nardoor
Copy link
Contributor Author

nardoor commented Oct 20, 2025

@aawsome

I would love to have feedback on the tests I added, this is the first time I do integration tests for rustic.
I took inspiration from other integration tests.

I first tried to use InMemoryBackend for those tests but couldn't get it to work. So I took a more abstract approach.

Also I noticed that rustic_core has a dev-dependency on rustic_testing.
However I observed that we can't really use InMemoryBackend in rustic_core unit tests (I tried) because:

  • rustic_testing also has a dependency on rustic_core
  • and using InMemoryBackend WriteBackend methods in a rustic_core unit test resulted in a compilation error about Multiple definitions for WriteBackend (one in the rustic_core crate, another in the rustic_core dependency of rustic_testing (those are the same project (same filesystem path), but I believe the circular dependency just doesn't cut it)

@aawsome
Copy link
Member

aawsome commented Oct 23, 2025

I would love to have feedback on the tests I added, this is the first time I do integration tests for rustic. I took inspiration from other integration tests.

The tests are OK from my POV. You could try to only once create a repo containing snapshots for all test cases and then run all tests (which do not modify the repo) on them, that would simplify the tests and speed them up a bit. But I am also fine with leaving them as-is.
I approved the change, if you want to leave the tests, you can merge this PR.

aawsome
aawsome previously approved these changes Oct 23, 2025
Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot @nardoor

@nardoor
Copy link
Contributor Author

nardoor commented Oct 23, 2025

The tests are OK from my POV. You could try to only once create a repo containing snapshots for all test cases and then run all tests (which do not modify the repo) on them, that would simplify the tests and speed them up a bit. But I am also fine with leaving them as-is. I approved the change, if you want to leave the tests, you can merge this PR.

Thanks for the feedback, I added a commit to apply your suggestion, I used a LazyLock . Maybe you had another way to do it in mind. If so I'm curious about it.

Otherwise I think we are pretty good to merge!

@nardoor nardoor force-pushed the feature/broad_snapshot_id branch 4 times, most recently from 03dbb3f to a4bd533 Compare October 23, 2025 22:40
@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from a4bd533 to 3982cda Compare October 23, 2025 22:42
@nardoor nardoor force-pushed the feature/broad_snapshot_id branch from 3982cda to 79289aa Compare October 23, 2025 22:48
@aawsome
Copy link
Member

aawsome commented Oct 24, 2025

I used a LazyLock . Maybe you had another way to do it in mind. If so I'm curious about it.

I think using #[fixture] (like in set_up_repo()) with #[once] from rstest (see https://docs.rs/rstest/latest/rstest/attr.fixture.html) should also do it.

@nardoor
Copy link
Contributor Author

nardoor commented Oct 24, 2025

I think using #[fixture] (like in set_up_repo()) with #[once] from rstest (see https://docs.rs/rstest/latest/rstest/attr.fixture.html) should also do it.

This makes total sense, I didn't know we could use once with rstest's fixtures.
I re-implemented the repo and snapshots setup :)

Note that:
I would have preferred that repo_and_snapshots returned a Result but I struggled to make it work.

  • using once is kind of like using a LazyLock, we can only get a reading ref on it
  • then I tried using Result::as_ref which works to get the inner result
  • but I couldn't manage to use the try operator, because we also have a ref on an anyhow error (which I tried mapping to something else but eventually gave up as it's "only tests")

Copy link
Member

@aawsome aawsome left a comment

Choose a reason for hiding this comment

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

LGTM

@aawsome aawsome added this pull request to the merge queue Oct 24, 2025
Merged via the queue into main with commit 6ef4d5a Oct 24, 2025
24 checks passed
@aawsome aawsome deleted the feature/broad_snapshot_id branch October 24, 2025 17:46
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.

2 participants