Unit test Fail when running UT_Culture and UT_RemoteNode together #922

eryeer · 2019-07-16T04:05:41Z

We encountered a strange bug. We were able to successfully run all the unit tests yesterday, but today unit test result is failed when running UT_RemoteNode and UT_Culture together on some computers (and some other computers can run successfully). Recently this part of code seems to have not been modified. The problem is akka error.

When running RemoteNode_Test_Accept_IfSameMagic method alone, the result is success.

When running All_Tests_Cultures method alone, the result is failed.

The text was updated successfully, but these errors were encountered:

lock9 · 2019-07-16T04:45:50Z

I think we have a mailbox issue. I don't understand the changes Erik made to make the mailbox work. Can you disable these tests?

eryeer · 2019-07-16T05:15:28Z

OK, let's disable them first.

igormcoelho · 2019-07-17T18:06:31Z

I'll try to understand this too, it's fundamental for my understanding of other Akka/TestKit features.. this is supposed to work properly. 🤔

lock9 · 2019-07-18T20:21:26Z

@igormcoelho Maybe we should reset the akka system during test initialization? Not sure, I think it is something with the mailbox, I can't be sure.
The problem is: I proposed this PR with some different modifications that the team didn't like much (like the usage of a factory), however, that was the recommended way of doing. I don't have idea of what could be done to replace it :(

igormcoelho · 2019-07-18T20:27:23Z

We need to understand the real cause first.

First, we need to make sure every TestKit instance is independent. It should be.
Second, if it's not independent, by nature, then we should find a way of completely serializing all unit tests (because we won't find any solution to this). But I think it shouldn't be designed for that.
Third, if it's supposed to be independent (as I think), but it's not being completely independent, it means some things are shared, that shouldn't. And I suspect one of the shared things (that shouldn't) is precisely the mailbox. We pass a configuration string on the real ActorSystem, and perhaps we shouldn't reuse same string for mailbox on dummy ActorSystem (TestKit), otherwise they may point to the same instance (real and fake), just a huge guess here.

Perhaps we need to change a few things on the code, perhaps controlling this string in a single place is enough to avoid problems. Let's see.

igormcoelho · 2019-07-18T20:38:58Z

It's systematically failing here, and systematically passing when I repeat it (only it). It must be some shared resources.

igormcoelho · 2019-07-18T22:05:34Z

Looks like it's building Connection twice 🤔

Failed   RemoteNode_Test_Accept_IfSameMagic_2
Error Message:
 Test method Neo.UnitTests.UT_RemoteNode.RemoteNode_Test_Accept_IfSameMagic_2 threw exception: 
Xunit.Sdk.TrueException: Failed: Expected a message of type Akka.IO.Tcp+Write, but received {Akka.IO.Tcp+Close} (type Akka.IO.Tcp+Close) instead  from TestActor[akka://test/user/$$j]
Expected: True
Actual:   False
Stack Trace:
    at Xunit.Assert.True(Nullable`1 condition, String userMessage) in C:\Dev\xunit\xunit\src\xunit.assert\Asserts\BooleanAsserts.cs:line 95
   at Akka.TestKit.TestKitBase.InternalExpectMsgEnvelope[T](Nullable`1 timeout, Action`2 assert, String hint, Boolean shouldLog)
   at Akka.TestKit.TestKitBase.InternalExpectMsgEnvelope[T](Nullable`1 timeout, Action`1 msgAssert, Action`1 senderAssert, String hint)
   at Akka.TestKit.TestKitBase.InternalExpectMsg[T](Nullable`1 timeout, Action`1 msgAssert, String hint)
   at Akka.TestKit.TestKitBase.ExpectMsg[T](Nullable`1 duration, String hint)
   at Neo.UnitTests.UT_RemoteNode.RemoteNode_Test_Accept_IfSameMagic_2() in /home/imcoelho/git-reps/neo-tests/docker-configure-neo/neo-core/neo.UnitTests/UT_RemoteNode.cs:line 110

Standard Output Messages:
 Started build Connection!
 Crazy TCP!!
 Finished build Connection!
 Started build RemoteNode!
 Finished build RemoteNode! Will send data!
 Aqui!!!!
 Started build Connection!
 Crazy TCP!!
 Finished build Connection!
 Started build RemoteNode!
 Finished build RemoteNode! Will send data!
 [ERROR][18/07/2019 22:04:03][Thread 0004][akka://test/user/$$j] Object reference not set to an instance of an object.
 Cause: System.NullReferenceException: Object reference not set to an instance of an object.
    at Neo.Network.P2P.RemoteNode.<>c__DisplayClass39_0.<OnVersionPayload>b__1(RemoteNode p) in /home/imcoelho/git-reps/neo-tests/docker-configure-neo/neo-core/neo/Network/P2P/RemoteNode.cs:line 217
    at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source, Func`2 predicate)
    at Neo.Network.P2P.RemoteNode.OnVersionPayload(VersionPayload version) in /home/imcoelho/git-reps/neo-tests/docker-configure-neo/neo-core/neo/Network/P2P/RemoteNode.cs:line 217
    at Neo.Network.P2P.RemoteNode.OnReceive(Object message) in /home/imcoelho/git-reps/neo-tests/docker-configure-neo/neo-core/neo/Network/P2P/RemoteNode.cs:line 137
    at Akka.Actor.UntypedActor.Receive(Object message)
    at Akka.Actor.ActorBase.AroundReceive(Receive receive, Object message)
    at Akka.Actor.ActorCell.ReceiveMessage(Object message)
    at Akka.Actor.ActorCell.Invoke(Envelope envelope)

lock9 · 2019-07-18T22:07:07Z

@igormcoelho do you think this may be evidence that we have duplicated messages? Can you check? I'm using my mac right now, I can't run tests 😞

igormcoelho · 2019-07-18T23:22:51Z

Looks like inspecting from Remote.Address (in RemoteNode class) causes another Connection to emerge... still looking into it. I don't know if it's a problem.

...Any(p => p.Remote.Address.Equals(Remote.Address) && p.Version?.Nonce == version.Nonce))

Seems connected to this issue:
Good execution:

 Started build Connection! TestProbe([akka://test/system/testActor35#572150908])
 Crazy TCP!!
 Finished build Connection!
 Started build RemoteNode!
 Finished build RemoteNode! Will send data!
 RemoteNode: OnReceive message
 On Version Payload (after OnReceived)
 Aqui!!!!
 Vai conferir Nonce!!!!
 Vai conferir Magic!!!!
 Vai comp ||
 Aqui2!!!!
 FORÇANDO BUG!

Bad execution:

 * Creating RemoteNode actor on tests
 Started build Connection! TestProbe([akka://test/system/MyTestProbeTCP#190355701])
 Crazy TCP!!
 Finished build Connection!
 Started build RemoteNode!
 Finished build RemoteNode! Will send data!
  -> Will expect message (Tcp.Write)
  <- Message arrived! (Tcp.Write)
  -> Will send a payload to remote node
 RemoteNode: OnReceive message
 On Version Payload (after OnReceived)
 Aqui!!!!
 Vai conferir Nonce!!!!
 Vai conferir Magic!!!!
 Vai comp ||
 Vai outra doideira!!!!
 Vai outra doideira x2!!!!
 Vai outra doideira x3!!!!
  <- Payload sent to remote node
  -> Will expect a Tcp.Write again!
 Started build Connection! TestProbe([akka://test/system/MyTestProbeTCP#190355701])
 Crazy TCP!!
 Finished build Connection!
 Started build RemoteNode!
 Finished build RemoteNode! Will send data!
 [ERROR][18/07/2019 23:24:45][Thread 0009][akka://test/user/$$i] Object reference not set to an instance of an object.

By simply inspecting on Remote.Address, all tests break now (DifferentMagic would never check it, that's why they pass..). In fact, Remote is null hahahah it should throw a NullPointerException... test is broken indeed ;) needs to fix this.

var remoteNodeActor = ActorOfAsTestActorRef(() => new RemoteNode(testBlockchain, connectionTestProbe, null, null));

These null cannot exist.

lock9 · 2019-07-19T00:04:24Z

@igormcoelho but I still don't understand why this caused the side effects, but I trust you when you say it is the test. Why does it use to work?

igormcoelho · 2019-07-19T01:20:23Z

I'm nearly fixing it in fact... another issue now is that:

LocalNode.Singleton.RemoteNodes.Values.Where(p => p != this)

is not cutting itself I guess...

========

I think I discovered the reason (another issue, besides the null). When you run multiple tests, you record a new RemoteNode on LocalNode.Singleton.RemoteNodes.Values, which is shared among all test instances. So, eventually, situation changes as more tests gets executed. It would be nice to deal with specific pools of RemoteNodes on each test, independently of others. Meaning that we need to improve this Singleton manner on LocalNode.

shargon · 2019-07-19T08:03:18Z

i receive this one

Message: Test method Neo.UnitTests.UT_Culture.All_Tests_Cultures threw exception: 
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> Xunit.Sdk.TrueException: Failed: Expected a message of type Akka.IO.Tcp+Write, but received {Akka.IO.Tcp+Close} (type Akka.IO.Tcp+Close) instead  from TestActor[akka://test/user/$$l]
Expected: True
Actual:   False

From this RemoteNode_Test_Accept_IfSameMagic

shargon · 2019-07-19T08:08:00Z

We need that testBlockchain = TestBlockchain.InitializeMockNeoSystem(); is not shared between classes, I think It is important for parallel tests.

igormcoelho · 2019-07-19T14:34:08Z

after this weekend I think I can propose a good solution for this, definitive one. Now that the cause is understood...

shargon · 2019-07-19T15:35:00Z

please review this until that #936

vncoelho · 2019-08-29T07:48:16Z

@igormcoelho, can you please come back with a good solution that has a strategical design?

igormcoelho · 2019-08-29T07:49:29Z

I fully agree, just should be properly fixed, not just workarounds, one after another. This affects all tests involving blockchain test module, for all other modules.
Better leave this closed, and start another one from scratch now.

lock9 added the Bug Used to tag confirmed bugs label Jul 16, 2019

lock9 self-assigned this Jul 16, 2019

shargon added a commit to shargon/neo that referenced this issue Jul 19, 2019

Temporary fix for neo-project#922

deae08c

shargon mentioned this issue Jul 19, 2019

Temporary fix for #922 #936

Merged

shargon closed this as completed in #936 Jul 19, 2019

shargon added a commit that referenced this issue Jul 19, 2019

Temporary fix for #922 (#936)

62766eb

igormcoelho mentioned this issue Aug 29, 2019

Unit test For Legder module #1038

Merged

vncoelho reopened this Aug 29, 2019

igormcoelho closed this as completed Aug 29, 2019

igormcoelho mentioned this issue Aug 29, 2019

Isolate unit tests #1019

Closed

Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020

Remove Nest (neo-project#922)

4e4533f

Tommo-L pushed a commit to Tommo-L/neo that referenced this issue Jun 22, 2020

Temporary fix for neo-project#922 (neo-project#936)

deb3e6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test Fail when running UT_Culture and UT_RemoteNode together #922

Unit test Fail when running UT_Culture and UT_RemoteNode together #922

eryeer commented Jul 16, 2019 •

edited

Loading

lock9 commented Jul 16, 2019

eryeer commented Jul 16, 2019

igormcoelho commented Jul 17, 2019

lock9 commented Jul 18, 2019

igormcoelho commented Jul 18, 2019 •

edited

Loading

igormcoelho commented Jul 18, 2019 •

edited

Loading

igormcoelho commented Jul 18, 2019

lock9 commented Jul 18, 2019

igormcoelho commented Jul 18, 2019 •

edited

Loading

lock9 commented Jul 19, 2019

igormcoelho commented Jul 19, 2019 •

edited

Loading

shargon commented Jul 19, 2019 •

edited

Loading

shargon commented Jul 19, 2019

igormcoelho commented Jul 19, 2019 •

edited

Loading

shargon commented Jul 19, 2019

vncoelho commented Aug 29, 2019

igormcoelho commented Aug 29, 2019 •

edited

Loading

Unit test Fail when running UT_Culture and UT_RemoteNode together #922

Unit test Fail when running UT_Culture and UT_RemoteNode together #922

Comments

eryeer commented Jul 16, 2019 • edited Loading

lock9 commented Jul 16, 2019

eryeer commented Jul 16, 2019

igormcoelho commented Jul 17, 2019

lock9 commented Jul 18, 2019

igormcoelho commented Jul 18, 2019 • edited Loading

igormcoelho commented Jul 18, 2019 • edited Loading

igormcoelho commented Jul 18, 2019

lock9 commented Jul 18, 2019

igormcoelho commented Jul 18, 2019 • edited Loading

lock9 commented Jul 19, 2019

igormcoelho commented Jul 19, 2019 • edited Loading

shargon commented Jul 19, 2019 • edited Loading

shargon commented Jul 19, 2019

igormcoelho commented Jul 19, 2019 • edited Loading

shargon commented Jul 19, 2019

vncoelho commented Aug 29, 2019

igormcoelho commented Aug 29, 2019 • edited Loading

eryeer commented Jul 16, 2019 •

edited

Loading

igormcoelho commented Jul 18, 2019 •

edited

Loading

igormcoelho commented Jul 18, 2019 •

edited

Loading

igormcoelho commented Jul 18, 2019 •

edited

Loading

igormcoelho commented Jul 19, 2019 •

edited

Loading

shargon commented Jul 19, 2019 •

edited

Loading

igormcoelho commented Jul 19, 2019 •

edited

Loading

igormcoelho commented Aug 29, 2019 •

edited

Loading