{{ message }}
Some test_mksession tests do not cleanup all the state#20691
Closed
illia-bobyr wants to merge 1 commit into
Closed
Some test_mksession tests do not cleanup all the state#20691illia-bobyr wants to merge 1 commit into
illia-bobyr wants to merge 1 commit into
Conversation
illia-bobyr
force-pushed
the
pr/test_mksession-cleanup-dependencies
branch
from
f21d6e7 to
8fc4b5e
Compare
Problem: Some test_mksession tests do not cleanup all the state Solution: Add commands to clean up state introduced by the test Before this change the following 4 tests were failing when executed individually: Test_mksession_arglocal_localdir Test_mksession_buffer_count Test_mksession_one_buffer_two_windows Test_mksession_winminheight As in ``` TEST_FILTER=winminheight make test_mksession ``` Yet, when ran as part of the whole test suite they succeeded. This was due to some state leaking from one test into another. I think this is bad, as it can confuse someone making changes in the relevant area. `Test_mksession_winminheight` is actually still broken a bit, and requires `winheight` and `winwidth` set at the beginning of the test, rather than later, when it actually matters. This exposes a subtle bug in the session restore script. I have a patch in a separate commit.
illia-bobyr
force-pushed
the
pr/test_mksession-cleanup-dependencies
branch
from
8fc4b5e to
60411c6
Compare
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem: Some test_mksession tests do not cleanup all the state
Solution: Add commands to clean up state introduced by the test
Before this change the following 4 tests were failing when executed individually:
Test_mksession_arglocal_localdir
Test_mksession_buffer_count
Test_mksession_one_buffer_two_windows
Test_mksession_winminheight
As in
Yet, when ran as part of the whole test suite they succeeded.
This was due to some state leaking from one test into another.
I think this is bad, as it can confuse someone making changes in the relevant area.
Test_mksession_winminheightis actually still broken a bit, and requireswinheightandwinwidthset at the beginning of the test, rather than later, when it actually matters. This exposes a subtle bug in the session restore script. I have a patch in a separate commit.Reviewer note.
Not sure if the change in
Test_mksession_localmappings()is justified.The minimum change would be to just add
set sessionoptions&at the end.But a
try/finallyblock makes sure that a failure would not prevent the cleanup from happening.I do not know if this is an issue in the first place.
I can see that
try/finallyis used in the test, but not a lot.