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.

Sign up for GitHub

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

Jump to bottom

Speed up CI tests, replace delays with "signalling" #4244

Closed
kiminuo opened this issue Aug 27, 2020 · 6 comments
Closed

Speed up CI tests, replace delays with "signalling" #4244

kiminuo opened this issue Aug 27, 2020 · 6 comments
Assignees
@kiminuo

Comments

@kiminuo
Copy link
Collaborator

kiminuo commented Aug 27, 2020 •
edited

This issue is part of #4152 project.

I have searched for Task.Delay( in the WalletWasabi.Tests project in VS Code. The results follow. It's moderate amount of results and I would like each occurrence either mark as "OK" or "makes tests unstable" (which would require a PR for fixing that).

IntegrationTests\LiveServerTests.cs:
  29  			torManager.Start(ensureRunning: true, dataDir: Path.GetFullPath(AppContext.BaseDirectory));
  30: 			Task.Delay(3000).GetAwaiter().GetResult();
  31  		}

IntegrationTests\TorTests.cs:
  20  			torManager.Start(ensureRunning: true, dataDir: Path.GetFullPath(AppContext.BaseDirectory));
  21: 			Task.Delay(3000).GetAwaiter().GetResult();
  22  		}

PeriodicRunnerTests\SerialPeriodicRunnerTests.cs:
  32  				// Add some delay to simulate work.
  33: 				await Task.Delay(50, cancel).ConfigureAwait(false);
  34  				RoundCounter++;

RegressionTests\BackendTests.cs:
  151  					}
  152: 					await Task.Delay(100);
  153  					times++;

  165  					}
  166: 					await Task.Delay(100);
  167  					times++;

  216  					}
  217: 					await Task.Delay(100);
  218  					times++;

RegressionTests\BuildTests.cs:
  159  				var txId2 = await rpc.SendToAddressAsync(keyManager.GetNextReceiveKey("bar", out _).GetP2wpkhAddress(network), Money.Coins(2m));
  160: 				await Task.Delay(1000); // Wait tx to arrive and get processed.
  161  

  394  				var fundingBumpTxId = await rpc.BumpFeeAsync(fundingTxId);
  395: 				await Task.Delay(2000); // Waits for the funding transaction get to the mempool.
  396  				Assert.Contains(fundingBumpTxId.TransactionId, wallet.Coins.Select(x => x.TransactionId));

RegressionTests\CoinJoinTests.cs:
   401  			// Wait until input registration times out.
   402: 			await Task.Delay(TimeSpan.FromSeconds(8));
   403  			httpRequestException = await Assert.ThrowsAsync<HttpRequestException>(async () => await CreateNewAliceClientAsync(roundId, registeredAddresses, schnorrPubKeys, requesters, inputsRequest));

  1086  
  1087: 				Task timeout = Task.Delay(TimeSpan.FromSeconds(connectionConfirmationTimeout * 2 + 7 * 2 + 7 * 2 + 7 * 2));
  1088  				while ((await rpc.GetRawMempoolAsync()).Length == 0)

  1094  
  1095: 					await Task.Delay(1000);
  1096  				}

  1104  
  1105: 					Task timeout = Task.Delay(3000);
  1106  					while (chaumianClient.State.GetActivelyMixingRounds().Any())

  1111  						}
  1112: 						await Task.Delay(1000);
  1113  					}

  1218  
  1219: 				Task timeout = Task.Delay(TimeSpan.FromSeconds(connectionConfirmationTimeout * 2 + 7 * 2 + 7 * 2 + 7 * 2));
  1220  				while ((await rpc.GetRawMempoolAsync()).Length == 0)

  1225  					}
  1226: 					await Task.Delay(1000);
  1227  				}

  1247  				{
  1248: 					await Task.Delay(1000);
  1249  				}

  1255  				{
  1256: 					await Task.Delay(1000);
  1257  					if (times > 21)

  1378  				{
  1379: 					await Task.Delay(1000);
  1380  					waitCount++;

  1388  				{
  1389: 					await Task.Delay(1000);
  1390  					waitCount++;

  1399  
  1400: 				Task timeout = Task.Delay(TimeSpan.FromSeconds(2 * (1 + 11 + 7 + 3 * (3 + 7))));
  1401  				while (wallet.Coins.Count() != 4)

  1406  					}
  1407: 					await Task.Delay(1000);
  1408  				}

  1412  				{
  1413: 					await Task.Delay(1000);
  1414  					times++;

  1431  					{
  1432: 						await Task.Delay(1000);
  1433  					}

RegressionTests\Common.cs:
  38  				}
  39: 				await Task.Delay(TimeSpan.FromSeconds(1));
  40  				times++;

  72  				{
  73: 					await Task.Delay(100);
  74  				}

RegressionTests\DosTests.cs:
   73  			{
   74: 				await Task.Delay(100);
   75  				if (times > 50) // 5 sec, 3 should be enough

  213  				{
  214: 					await Task.Delay(100);
  215  					if (times > 50) // 5 sec, 3 should be enough

  266  				{
  267: 					await Task.Delay(100);
  268  					if (times > 50) // 5 sec, 3 should be enough

RegressionTests\SendTests.cs:
   98  				{
   99: 					await Task.Delay(1000);
  100  					waitCount++;

  750  				{
  751: 					await Task.Delay(500); // Waits for the funding transaction get to the mempool.
  752  				}

  758  				var tx1Id = bfr.TransactionId;
  759: 				await Task.Delay(2000); // Waits for the replacement transaction get to the mempool.
  760  				Assert.Single(wallet.Coins);

  765  				var tx2Id = bfr.TransactionId;
  766: 				await Task.Delay(2000); // Waits for the replacement transaction get to the mempool.
  767  				Assert.Single(wallet.Coins);

RegressionTests\WalletTests.cs:
   76  				}
   77: 				await Task.Delay(TimeSpan.FromSeconds(1));
   78  				times++;

  101  					}
  102: 					await Task.Delay(100);
  103  					times++;

  116  					}
  117: 					await Task.Delay(100);
  118  					times++;

  428  				var txId5 = await rpc.SendToAddressAsync(key.GetP2wpkhAddress(network), Money.Coins(0.1m));
  429: 				await Task.Delay(1000); // Wait tx to arrive and get processed.
  430  				Assert.NotEmpty(wallet.Coins.Where(x => x.TransactionId == txId5));

UnitTests\AsyncMutexTests.cs:
   77  			{
   78: 				await Task.Delay(1);
   79  			}

   82  			{
   83: 				await Task.Delay(1);
   84  			}

   95  			{
   96: 				await Task.Delay(1);
   97  			}

  108  
  109: 					await Task.Delay(rand.Next(5));
  110  					numbers.Add(cnt);

UnitTests\IoTests.cs:
   81  			var currentDate = File.GetLastWriteTimeUtc(ioman1.FilePath);
   82: 			await Task.Delay(500);
   83  			await ioman1.WriteAllLinesAsync(lines);

   88  			currentDate = File.GetLastWriteTimeUtc(ioman1.FilePath);
   89: 			await Task.Delay(500);
   90  			lines.Add("Lorem ipsum dolor sit amet, consectetur adipiscing elit.");

  264  			t3.Start();
  265: 			await Task.Delay(100);
  266  			t1.Join();

UnitTests\MemoryTests.cs:
   31  
   32: 			await Task.Delay(TimeSpan.FromMilliseconds(20));
   33  

  148  
  149: 			await Task.Delay(1);
  150  

UnitTests\SerialBlockNotifierTests.cs:
  117  			// Waits at most 1.5s given CancellationTokenSource definition
  118: 			await Task.WhenAny(Task.Delay(Timeout.InfiniteTimeSpan, cts.Token));
  119  

  286  			{
  287: 				await Task.Delay(TimeSpan.FromSeconds(1));
  288  			}

UnitTests\SmartBlockProviderTests.cs:
  56  				// This does simulate work necessary to actually fetch the block.
  57: 				await Task.Delay(TimeSpan.FromSeconds(0.5));
  58  				return Blocks[hash];

UnitTests\BitcoinCore\IndexBuilderServiceTests.cs:
   33  
   34: 			await Task.Delay(TimeSpan.FromSeconds(1));
   35  			//// Assert.False(indexer.IsRunning);     // <------------ ERROR: it should have stopped but there is a bug for RegTest

   60  
   61: 			await Task.Delay(TimeSpan.FromSeconds(2));
   62  			Assert.True(indexer.IsRunning);  // It is still working

   91  
   92: 			await Task.Delay(TimeSpan.FromSeconds(10));
   93  			Assert.True(indexer.IsRunning);  // It is still working

  117  
  118: 			await Task.Delay(TimeSpan.FromSeconds(5));
  119  			Assert.False(indexer.IsRunning);  // we are done

XunitConfiguration\RegTestFixture.cs:
  75  
  76: 			var delayTask = Task.Delay(3000);
  77  			Task.WaitAny(delayTask, hostInitializationTask); // Wait for server to initialize (Without this OSX CI will fail)
@molnard
Copy link
Collaborator

molnard commented Aug 27, 2020

Shall I make those notes here?

@kiminuo
Copy link
Collaborator Author

kiminuo commented Aug 27, 2020 •
edited

@molnard Sure, you can.

@kiminuo kiminuo mentioned this issue Aug 27, 2020
Merged
@molnard
Copy link
Collaborator

molnard commented Aug 27, 2020

This issue is part of #4152 project.

I have searched for Task.Delay( in the WalletWasabi.Tests project in VS Code. The results follow. It's moderate amount of results and I would like each occurrence either mark as "OK" or "makes tests unstable" (which would require a PR for fixing that).

bin\Debug\netcoreapp3.1\WalletWasabi.xml:
  2547                  {
  2548:                     await Task.Delay(TimeSpan.FromSeconds(1));
  2549                  }

WalletWasabi.xml?

IntegrationTests\LiveServerTests.cs:
29 torManager.Start(ensureRunning: true, dataDir: Path.GetFullPath(AppContext.BaseDirectory));
30: Task.Delay(3000).GetAwaiter().GetResult();
31 }

Probably this can be sped up.

IntegrationTests\TorTests.cs:
20 torManager.Start(ensureRunning: true, dataDir: Path.GetFullPath(AppContext.BaseDirectory));
21: Task.Delay(3000).GetAwaiter().GetResult();
22 }

Probably this can be sped up.

PeriodicRunnerTests\SerialPeriodicRunnerTests.cs:
32 // Add some delay to simulate work.
33: await Task.Delay(50, cancel).ConfigureAwait(false);
34 RoundCounter++;

Try to reduce the delay to 1 ms. That will ensure Threadpool execution and the less delay possible. I am not sure about this have to try.

RegressionTests\BackendTests.cs:
151 }
152: await Task.Delay(100);
153 times++;

165 }
166: await Task.Delay(100);
167 times++;

216 }
217: await Task.Delay(100);
218 times++;

This is good, these are polling intervals. Can you change the timeout logic that is using TimeSpan and StopWatch of DateTimeOffset?

RegressionTests\BuildTests.cs:
159 var txId2 = await rpc.SendToAddressAsync(keyManager.GetNextReceiveKey("bar", out _).GetP2wpkhAddress(network), Money.Coins(2m));
160: await Task.Delay(1000); // Wait tx to arrive and get processed.
161

It depends, can we detect if tx in the mempool?

394 var fundingBumpTxId = await rpc.BumpFeeAsync(fundingTxId);
395: await Task.Delay(2000); // Waits for the funding transaction get to the mempool.
396 Assert.Contains(fundingBumpTxId.TransactionId, wallet.Coins.Select(x => x.TransactionId));

It depends, can we detect if tx in the mempool?

RegressionTests\CoinJoinTests.cs:
401 // Wait until input registration times out.
402: await Task.Delay(TimeSpan.FromSeconds(8));
403 httpRequestException = await Assert.ThrowsAsync(async () => await CreateNewAliceClientAsync(roundId, registeredAddresses, schnorrPubKeys, requesters, inputsRequest));

It depends, are we the ones who set 8 sec to registration timeout?

1086
1087: Task timeout = Task.Delay(TimeSpan.FromSeconds(connectionConfirmationTimeout * 2 + 7 * 2 + 7 * 2 + 7 * 2));
1088 while ((await rpc.GetRawMempoolAsync()).Length == 0)

1094
1095: await Task.Delay(1000);
1096 }

Strange timeout detection, also I would reduce the poll interval to 100 ms.

1104
1105: Task timeout = Task.Delay(3000);
1106 while (chaumianClient.State.GetActivelyMixingRounds().Any())

1111 }
1112: await Task.Delay(1000);
1113 }

reduce the poll interval to 100 ms.

1218
1219: Task timeout = Task.Delay(TimeSpan.FromSeconds(connectionConfirmationTimeout * 2 + 7 * 2 + 7 * 2 + 7 * 2));
1220 while ((await rpc.GetRawMempoolAsync()).Length == 0)

1225 }
1226: await Task.Delay(1000);
1227 }

reduce the poll interval to 100 ms.

1247 {
1248: await Task.Delay(1000);
1249 }

1255 {
1256: await Task.Delay(1000);
1257 if (times > 21)

1378 {
1379: await Task.Delay(1000);
1380 waitCount++;

1388 {
1389: await Task.Delay(1000);
1390 waitCount++;

1399
1400: Task timeout = Task.Delay(TimeSpan.FromSeconds(2 * (1 + 11 + 7 + 3 * (3 + 7))));
1401 while (wallet.Coins.Count() != 4)

1406 }
1407: await Task.Delay(1000);
1408 }

1412 {
1413: await Task.Delay(1000);
1414 times++;

1431 {
1432: await Task.Delay(1000);
1433 }

Same for all, reduce the poll interval to 100 ms.

RegressionTests\Common.cs:
38 }
39: await Task.Delay(TimeSpan.FromSeconds(1));
40 times++;

72 {
73: await Task.Delay(100);
74 }

Use a better timeout mechanism and reduce polling interval to 100ms

RegressionTests\DosTests.cs:
73 {
74: await Task.Delay(100);
75 if (times > 50) // 5 sec, 3 should be enough

213 {
214: await Task.Delay(100);
215 if (times > 50) // 5 sec, 3 should be enough

266 {
267: await Task.Delay(100);
268 if (times > 50) // 5 sec, 3 should be enough

RegressionTests\SendTests.cs:
98 {
99: await Task.Delay(1000);
100 waitCount++;

750 {
751: await Task.Delay(500); // Waits for the funding transaction get to the mempool.
752 }

758 var tx1Id = bfr.TransactionId;
759: await Task.Delay(2000); // Waits for the replacement transaction get to the mempool.
760 Assert.Single(wallet.Coins);

765 var tx2Id = bfr.TransactionId;
766: await Task.Delay(2000); // Waits for the replacement transaction get to the mempool.
767 Assert.Single(wallet.Coins);

Use a better timeout mechanism, detect mempool if possible

RegressionTests\WalletTests.cs:
76 }
77: await Task.Delay(TimeSpan.FromSeconds(1));
78 times++;

101 }
102: await Task.Delay(100);
103 times++;

116 }
117: await Task.Delay(100);
118 times++;

428 var txId5 = await rpc.SendToAddressAsync(key.GetP2wpkhAddress(network), Money.Coins(0.1m));
429: await Task.Delay(1000); // Wait tx to arrive and get processed.
430 Assert.NotEmpty(wallet.Coins.Where(x => x.TransactionId == txId5));
Use a better timeout mechanism, detect mempool if possible

UnitTests\AsyncMutexTests.cs:
77 {
78: await Task.Delay(1);
79 }

82 {
83: await Task.Delay(1);
84 }

95 {
96: await Task.Delay(1);
97 }

108
109: await Task.Delay(rand.Next(5));
110 numbers.Add(cnt);

Seems to be good.

UnitTests\IoTests.cs:
81 var currentDate = File.GetLastWriteTimeUtc(ioman1.FilePath);
82: await Task.Delay(500);
83 await ioman1.WriteAllLinesAsync(lines);

88 currentDate = File.GetLastWriteTimeUtc(ioman1.FilePath);
89: await Task.Delay(500);
90 lines.Add("Lorem ipsum dolor sit amet, consectetur adipiscing elit.");

Hmm why waiting here? It depends on the precision of GetLastWriteTimeUtc. Need to test it.

264 t3.Start();
265: await Task.Delay(100);
266 t1.Join();

Probably this can be improved, low prio.

UnitTests\MemoryTests.cs:
31
32: await Task.Delay(TimeSpan.FromMilliseconds(20));
33

148
149: await Task.Delay(1);
150

Waiting for expiration looks good.

UnitTests\SerialBlockNotifierTests.cs:
117 // Waits at most 1.5s given CancellationTokenSource definition
118: await Task.WhenAny(Task.Delay(Timeout.InfiniteTimeSpan, cts.Token));
119

286 {
287: await Task.Delay(TimeSpan.FromSeconds(1));
288 }

idk might be OK.

UnitTests\SmartBlockProviderTests.cs:
56 // This does simulate work necessary to actually fetch the block.
57: await Task.Delay(TimeSpan.FromSeconds(0.5));
58 return Blocks[hash];

UnitTests\BitcoinCore\IndexBuilderServiceTests.cs:
33
34: await Task.Delay(TimeSpan.FromSeconds(1));
35 //// Assert.False(indexer.IsRunning); // <------------ ERROR: it should have stopped but there is a bug for RegTest

60
61: await Task.Delay(TimeSpan.FromSeconds(2));
62 Assert.True(indexer.IsRunning); // It is still working

Can we poll?

91
92: await Task.Delay(TimeSpan.FromSeconds(10));
93 Assert.True(indexer.IsRunning); // It is still working

Can we poll?

117
118: await Task.Delay(TimeSpan.FromSeconds(5));
119 Assert.False(indexer.IsRunning); // we are done

Can we poll?

XunitConfiguration\RegTestFixture.cs:
75
76: var delayTask = Task.Delay(3000);
77 Task.WaitAny(delayTask, hostInitializationTask); // Wait for server to initialize (Without this OSX CI will fail)

Can we detect instead of wait?

@kiminuo
Copy link
Collaborator Author

kiminuo commented Aug 29, 2020

#4245 is linked to this.

@kiminuo kiminuo self-assigned this Aug 29, 2020
This was referenced Sep 4, 2020
Closed
Merged
Merged
Merged
This was referenced Sep 14, 2020
Merged
Merged
Merged
Merged
Merged
Merged
@kiminuo
Copy link
Collaborator Author

kiminuo commented Sep 24, 2020

@kiminuo kiminuo mentioned this issue Mar 7, 2021
Merged
@kiminuo
Copy link
Collaborator Author

kiminuo commented Sep 6, 2021

@molnard I believe this issue should be closed. If there are some other flaky tests. We should file them to new issues, I guess.

@kiminuo kiminuo closed this as completed Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kiminuo kiminuo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@molnard @kiminuo