-
Notifications
You must be signed in to change notification settings - Fork 297
Hot Reload test fix #3018
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
base: main
Are you sure you want to change the base?
Hot Reload test fix #3018
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Outdated
Show resolved
Hide resolved
Aniruddh25
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
RubenCerna2079
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just need to address comments and change the PR description
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Outdated
Show resolved
Hide resolved
src/Service.Tests/Configuration/HotReload/ConfigurationHotReloadTests.cs
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Why make this change?
Closes #2010
Note that hot-reload of the graphQL schema is bugged currently, and tests are still ignored until this issue is resolved: #3019
What is this change?
As documented in the issue above, a number of hot-reload tests were failing due to relying on a simple timeout to allow the hot-reload to complete within tests. This creates a number of potential problems, including potential race conditions within the tests. We replace that strategy with a waiting process and change from syn to async in a number of functions. We also cleanup the config creation in the tests so that the new properties added since these tests were ignored fit into the flow and expectations of the tests.
How was this tested?
The previously ignored tests are now run and have the ignore tag removed.
Sample Request(s)
N/A