test: Bookmarked: add tests to Bookmarked functions#163
test: Bookmarked: add tests to Bookmarked functions#163JGBSouza wants to merge 1 commit intokworkflow:unstablefrom
Conversation
JGBSouza
changed the title
JGBSouza
force-pushed
the
test/bookmarked_patches
branch
from
d4aba9c to
3846fb0
Compare
lorenzoberts
left a comment
lorenzoberts
left a comment
There was a problem hiding this comment.
Hey @JGBSouza, thanks for one more contribution.
- You can see now that some actions are failing, and that's because for some reason i still have to approve the workflows from you PR, so when you pushed it the actions were not running. Please fix the format and linting stuff. Also, the pre-commit should have failed when you commited these changes, so make sure it's running so we don't depend on github actions to run fmt, lint and tests
- We don't need to prefixes for the commit title, just the conventional commit prefix is sufficient. So instead of "test: Bookmarked: ...." you should replace with "test: add tests to...". This is a change i could make myself when pushing it to upstream, but i'm letting you know anyway.
- Let me know if my comments make sense. Nothing is super strict, but i see more as good practices when creating tests.
src/app/screens/bookmarked.rs
Outdated
| /// # Tests | ||
| /// | ||
| /// [tests::test_select_below_patchset_increments_index] | ||
| /// [tests::test_select_below_patchset_stops_at_end] | ||
| /// [tests::test_select_above_patchset_decrements_index] | ||
| /// [tests::test_select_above_patchset_stops_at_start] | ||
| /// [tests::test_get_selected_patchset_returns_correct_patch] | ||
| /// [tests::test_bookmark_selected_patch_adds_new_patch] | ||
| /// [tests::test_bookmark_selected_patch_does_not_duplicate] | ||
| /// [tests::test_unbookmark_selected_patch_removes_patch] | ||
| /// [tests::test_unbookmark_selected_patch_ignores_missing_patch] |
There was a problem hiding this comment.
What's the objective of these docstrings? Maybe it's my limited Rust knowledge but i don't think this comments will do anything
src/app/screens/bookmarked.rs
Outdated
| /// [tests::test_bookmark_selected_patch_does_not_duplicate] | ||
| /// [tests::test_unbookmark_selected_patch_removes_patch] | ||
| /// [tests::test_unbookmark_selected_patch_ignores_missing_patch] | ||
| pub fn make_patch(title: &str) -> Patch { |
There was a problem hiding this comment.
why does it need to be pub?
src/app/screens/bookmarked.rs
Outdated
| s.select_below_patchset(); | ||
| assert_eq!(s.patchset_index, 1); |
There was a problem hiding this comment.
it would be more consistent to assert the current patchset_index before and after the select_below_patchset call. So the function behavior is explicit in the test
| s.select_above_patchset(); | ||
| assert_eq!(s.patchset_index, 0); |
There was a problem hiding this comment.
same thing here. We shouldn't need to see mock_bookmarked_patchsets implementation to understand what's the initial patchset_index. You can assert or set the patchset_index
src/app/screens/bookmarked.rs
Outdated
| #[test] | ||
| fn test_unbookmark_selected_patch_removes_patch() { | ||
| let mut s = mock_bookmarked_patchsets(); | ||
| let to_remove = make_patch("Patch 2"); |
There was a problem hiding this comment.
it would make more sense to set to_remove as s.bookmarked_patchsets[1] and then unbookmark it. make_patch("Patch 2") wouldn't necessarily be in the mocked struct. If to_remove was make_patch("Patch 9"), the function would still work because Patch 9 was never bookmarked in the first place
| let mut s = mock_bookmarked_patchsets(); | ||
| let missing = make_patch("Inexistent"); | ||
| s.unbookmark_selected_patch(&missing); | ||
| assert_eq!(s.bookmarked_patchsets.len(), 3); |
There was a problem hiding this comment.
Again, you should be asserting the len before the unbookmark_selected_patch operation so the test behavior is explicit
JGBSouza
force-pushed
the
test/bookmarked_patches
branch
from
3846fb0 to
838622a
Compare
This commit add tests to the Bookmarked functions. Signed-off-by: JGBSouza <joaosouzaaa12@usp.br>
JGBSouza
force-pushed
the
test/bookmarked_patches
branch
from
838622a to
acd6f8e
Compare
2 participants
This commit add tests to the Bookmarked functions, increasing it's test coverage.