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 undo tests #91

Merged
merged 6 commits into from
Mar 12, 2019
Merged

Fix undo tests #91

merged 6 commits into from
Mar 12, 2019

Conversation

amolenaar
Copy link
Member

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes

What is the current behavior?

Tests fail due to inconsistent transaction state.

Issue Number: #75

What is the new behavior?

Tests work due to restructured transaction test.

Other information

One might argue that the state that was left behind from the transaction test should be fixed as part of the entire test case. That the "stack" (static) class variable of the Transaction class should not be there. For now this fixes the problem and future design improvements can focus on removing the class-wide variable in a way that allows for easy testing and (more) reliable execution.

@amolenaar amolenaar requested a review from danyeaw March 11, 2019 19:38
This was referenced Mar 11, 2019
Copy link
Member

@danyeaw danyeaw left a comment

Choose a reason for hiding this comment

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

This is a really nice collection of refactoring, I think this really goes a long way to clean up the code base by dropping support for these older model versions 👍

@danyeaw danyeaw merged commit c040c97 into master Mar 12, 2019
@danyeaw danyeaw deleted the fix-undo-tests branch March 12, 2019 01:36
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.

None yet

2 participants