Added End-To-End Test for Datasource Hot-Reload #2453
Conversation
RubenCerna2079
requested review from
Aniruddh25,
seantleonard,
aaronburtle,
abhishekkumams,
ravishetye,
rohkhann,
neeraj-sharma2592,
sourabh1007 and
sezal98
as code owners
|
/azp run |
| GenerateConfigFile( | ||
| databaseType: expectedDatabaseType, | ||
| connectionString: expectedConnectionString); | ||
| System.Threading.Thread.Sleep(3000); |
There was a problem hiding this comment.
Instead of adding a timer here, why dont we check for the existence of the new file, and wait until the new file is generated? Adding timer to the test makes it flaky.
There was a problem hiding this comment.
The file is already created beforehand and we are only updating the file.
We use a timer in order to ensure that the server has enough time to hot-reload before moving on with the rest of the testing.
There was a problem hiding this comment.
I understand why you are using timer, but what if the update doesnt happen within 3 sec? can we instead check for the condition you expect to be true after the expiry of the timer instead of using a timer?
There was a problem hiding this comment.
If I remember correctly, DAB is not able to work on any requests until hot-reload is finished. If that is the case, by having a request with await then the test should wait until the request and hot-reload are completed.
(Please correct me if I am wrong on that)
There was a problem hiding this comment.
@RubenCerna2079 , what happens if we don't add a sleep, does it fail?
There was a problem hiding this comment.
@abhishekkumams, yes, since it does not give enough time for the hot-reload to take place
There was a problem hiding this comment.
Created an event that triggers when hot reload is completed.
This will be used in the tests to make them wait until hot reload finishes, before continuing with the tests.
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Show resolved
Hide resolved
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Show resolved
Hide resolved
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
| // Assert | ||
| Assert.AreNotEqual(previousDatabaseType, actualDatabaseType); | ||
| Assert.AreEqual(expectedDatabaseType, actualDatabaseType); | ||
| SqlTestHelper.PerformTestEqualJsonStrings(_bookDBOContents, reloadGQLContents.GetProperty("items").ToString()); |
There was a problem hiding this comment.
_bookDBOContents has values from MSSQL, currently all the DBs have the same values, so we don't know if it actually picked the value from MSSQL or the updated DB. Would you be able to add one row to each db with different values which we can use for more robust testing?
There was a problem hiding this comment.
Also Based on how our pipelines work, we do not support cross database access in single pipeline. But you can add another test which i mentioned in this issue itself.
|
Can you also add one more test where you update the connection string with invalid value and then change it to a valid value, with 200 response? |
I don't think we need this test, since the hot-reload has a last known good config file to go to when there is an error, so it will not change anything by trying to change the connection string to an invalid value. And a successful connection string change is already tested with the current test that I am working on. |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
|
/azp run |
Why make this change?
This change solves the issue #1866
What is this change?
Added an end-to-end test to ensure that DAB hot-reloads appropriately when there is a connection change from one database to another database (e.g. MSSQL to PostgreSQL).
In order to make tests related to hot reload less flaky, we stopped using the sleep() function and added an event in
FileSystemRuntimeConfigLoader, which triggers when hot reload is complete. This event is used by the tests in order to wait until hot reload finishes.How was this tested?