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

Reverting Snapshots commits at the end of UTs #1343

Closed
wants to merge 4 commits into from

Conversation

vncoelho
Copy link
Member

@vncoelho vncoelho commented Dec 9, 2019

There are still other places to be reverted,

@ixje @shargon @erikzhang,

Take at look at this example and, if correct, we may create a function on Tests Utils, for avoiding duplicating the code on every case it happens.

image

@erikzhang
Copy link
Member

I think the problem is Blockchain.Singleton. We should try to remove Blockchain.Singleton and use dependency injection. Then all the tests are isolated.

@Tommo-L
Copy link
Contributor

Tommo-L commented Dec 10, 2019

@rodoufu #1042

@ixje
Copy link
Contributor

ixje commented Dec 10, 2019

I think the problem is with

[TestInitialize]
public void TestSetup()
{
TestBlockchain.InitializeMockNeoSystem();
}

I think it is correct to use this in TestInitialize, the problem is that this doesn't seem to create a new fresh Blockchain instance. Each call (and thus each test) should get a new clean instance. It's probably best to use the In-Memory blockchain if that's already merged somewhere, otherwise you'll probably have to add [TestCleanup].

@vncoelho
Copy link
Member Author

I agree, @erikzhang, that DI could solve this in a more beautiful way.

However, even nowadays, it is possible to handle that. We should not have tests that are finishing with a modified status different than the original.

Thus, perhaps, until DI is not implemented we should fix the tests that are doing such behavior.

@rodoufu
Copy link

rodoufu commented Dec 11, 2019

@rodoufu #1042

The DI may help to improve this kind of initialization for the tests.
But it may be also necessary to mock some stuff, probably we are going to register the components on the container differently for some tests.

@vncoelho vncoelho closed this May 20, 2020
@vncoelho vncoelho deleted the reverting-snapshot-on-tests branch May 20, 2020 16:57
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

5 participants