Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a few minor issues with the jj prev tests #3430

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Conversation

emesterhazy
Copy link
Contributor

@emesterhazy emesterhazy commented Apr 2, 2024

This commit adds commit graphs to most of the tests for jj prev to make it
clearer where @ points before and after prev is run.

In addition, there were a couple of tests where the comments suggested the test
meant to have @ pointing to a specific commit, but it actually pointed to an
empty child of that commit.

This sort of issue also exists in test_prev_editing. The test is supposed to
check that --edit is implied if you run jj prev on an interior commit, but
it actually caused a new empty commit to be created since @ was sitting on a
tip commit.

@emesterhazy emesterhazy force-pushed the push-qwxwsrzwqtxy branch 2 times, most recently from ff6572d to 2e6ca9e Compare April 3, 2024 22:22
@emesterhazy emesterhazy marked this pull request as ready for review April 3, 2024 22:22
cli/tests/test_next_prev_commands.rs Outdated Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Outdated Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Show resolved Hide resolved
@emesterhazy emesterhazy changed the title Fix issues with the jj next and jj prev tests Fix a few minor issues with the jj prev tests Apr 4, 2024
@emesterhazy emesterhazy force-pushed the push-qwxwsrzwqtxy branch 2 times, most recently from 9696d67 to 73711de Compare April 4, 2024 17:41
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

There are things I do not fully understand why they're done and could please undo the changes which mention discardable commits as they're missleading.

cli/tests/test_next_prev_commands.rs Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Show resolved Hide resolved
cli/tests/test_next_prev_commands.rs Outdated Show resolved Hide resolved
@emesterhazy emesterhazy force-pushed the push-qwxwsrzwqtxy branch 2 times, most recently from 9f4db66 to 9fbfa8b Compare April 4, 2024 18:13
@emesterhazy
Copy link
Contributor Author

There are things I do not fully understand why they're done and could please undo the changes which mention discardable commits as they're missleading.

Done. I deleted everything that mentioned discardable commits.

@emesterhazy
Copy link
Contributor Author

Is there anything else we should change or is this ready now?

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

lgtm

This commit adds commit graphs to most of the tests for `jj prev` to make it
clearer where `@` points before and after `prev` is run.

In addition, there were a couple of tests where the comments suggested the test
meant to have `@` pointing to a specific commit, but it actually pointed to an
empty child of that commit.

This sort of issue also exists in `test_prev_editing`. The test is supposed to
check that `--edit` is implied if you run `jj prev` on an interior commit, but
it actually caused a new empty commit to be created since `@` was sitting on a
tip commit.
@emesterhazy emesterhazy merged commit cc6d290 into main Apr 8, 2024
16 checks passed
@emesterhazy emesterhazy deleted the push-qwxwsrzwqtxy branch April 8, 2024 13:12
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