Reduced standard time for tests #649
Conversation
|
I am definitely in favor of this. Unit tests should test the behavior of the code not really specific to a particular configuration. Integration tests are where specific configuration is tested, such as what is performed by NGD. Unit tests should be written to execute in optimal time, so they do not hold up CI. |
neo.UnitTests/UT_Consensus.cs
Outdated
| { | ||
| ProtocolSettings.Default.SecondsPerBlock.Should().Be(2); | ||
| } | ||
|
|
There was a problem hiding this comment.
This redundant test made everything fail now.. lol time to fix old tests.
| @@ -57,12 +57,13 @@ public void ConsensusService_Primary_Sends_PrepareRequest_After_OnStart() | |||
|
|
|||
| int timeIndex = 0; | |||
| var timeValues = new[] { | |||
| new DateTime(1968, 06, 01, 0, 0, 15, DateTimeKind.Utc), // For tests here | |||
| //new DateTime(1968, 06, 01, 0, 0, 15, DateTimeKind.Utc), // For tests here | |||
There was a problem hiding this comment.
I removed it from array and also one consuming test below, just to make it simpler... in fact, some parts of this test can be improved in the future, or even split into many smaller tests. After we start writing other tests, we will re-write this one from scratch.
The main reason is because time was going back, and I don't like this anymore 😄
There was a problem hiding this comment.
Can we still just remove the commented line altogether?
| @@ -93,7 +94,6 @@ public void ConsensusService_Primary_Sends_PrepareRequest_After_OnStart() | |||
| Console.WriteLine($"header {header} hash {header.Hash} timstamp {timestampVal}"); | |||
|
|
|||
| timestampVal.Should().Be(4244941696); //1968-06-01 00:00:00 | |||
| TimeProvider.Current.UtcNow.ToTimestamp().Should().Be(4244941711); //1968-06-01 00:00:15 | |||
There was a problem hiding this comment.
this consumption was removed here too... but I can make it back if you prefer. We will re-write everything here as soon as we decide upon these fast tests.
|
Ok, I agree with @erikzhang and @jsolman , now anything is attached to the time on protocol.json file. We can change it freely ;) Is that ok? |
neo.UnitTests/protocol.json
Outdated
| "127.0.0.1:20334", | ||
| "127.0.0.1:20335", | ||
| "127.0.0.1:20336", | ||
| "127.0.0.1:20337", |
There was a problem hiding this comment.
I just put localhost, to make sure it won't try to connect to any external ip during tests. Any suggestions?
There was a problem hiding this comment.
Does this test need to start additional nodes?
There was a problem hiding this comment.
Now that protocol.json is not required, we can remove most of the options, leaving only SecondsPerBlock.
There was a problem hiding this comment.
Thanks a lot for that Erik. I'll update the tests.
|
Ok, I just kept Block Time (2 seconds) and also StandbyValidators, because we are using these exact values on UT_Consensus. However, by doing that, I broke something. |
|
Ok, now it's working. I just left block time (2 seconds), because validators are being entered manually on UT_Consensus. is it good for you guys @erikzhang @shargon @jsolman ? |
neo.UnitTests/UT_ProtocolSettings.cs
Outdated
| [TestMethod] | ||
| public void Test_Standby_Validators() | ||
| { | ||
| // Using Random values |
There was a problem hiding this comment.
Is there a need to include all this commented out code now? Either add it later when it is used? Or add it now if it is ready?
| @@ -57,12 +57,13 @@ public void ConsensusService_Primary_Sends_PrepareRequest_After_OnStart() | |||
|
|
|||
| int timeIndex = 0; | |||
| var timeValues = new[] { | |||
| new DateTime(1968, 06, 01, 0, 0, 15, DateTimeKind.Utc), // For tests here | |||
| //new DateTime(1968, 06, 01, 0, 0, 15, DateTimeKind.Utc), // For tests here | |||
There was a problem hiding this comment.
Can we still just remove the commented line altogether?
`dotnet new library` generates an error with dotnet version 2.1.402
This is an alternative to: #647
This implies the we will always use standard lower times (2 or 3 seconds). On practice, it's nearly impossible to use 15 seconds for any practical tests. However, we must decide a direction: single unit tests or multiple unit tests.