-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Disable a flaky ADLS test for now #3135
Conversation
This change affects catalog functionality: * Add new package catalog item type * Update to allow for listing of common catalog items from a base database: - Table - TableValuedFunction - TableStatistics - Views Updates for tests and versions. comment out new tests for ADL doing this until i can run tests in my local environment. Update tests and recordings. Actually copying the right file to the right place. update changelog. Cleanup ADL after migration to dev17 This cleans up files that are no longer used and updates csproj files to remove commented out sections Added a number of null checks to ADLS client We were getting some null reference exceptions on occasion so ensuring none of these nested properties are null before indexing to them. Move package ID to the top to address PR.
Will need more time to investigate a possible solution. Might need to completely remove dependence on filesystem and come up with some way to remove extra threads from tests.
@@ -388,7 +388,7 @@ public void DataLakeUploader_ResumeUploadDownloadWithAllMissingFiles() | |||
/// <summary> | |||
/// Tests the resume upload when only some segments were uploaded previously with progress tracking enabled | |||
/// </summary> | |||
[Fact] | |||
[Fact(Skip = "Randomly failing on CI. Needs investigation from DataLakeStore team")] |
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.
@begoldsm If you plan to fix this in the next month, great. Please file an issue and reference the issue in the skip message. Otherwise, please delete this test
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.
@markcowl I probably won't fix it within the next month. If there is some way to flag this test as a local only test I would like to do that. Otherwise I will need to come up with a more comprehensive plan for fixing the unit tests here to not require local file system access or multiple threads.
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.
I would say, please delete the test in that case.
As per recommendation from Mark. Progress tracking is well tested elsewhere, and we also have a test that validates resume functionality without progress tracking, so we should still be covered.
@markcowl done, I got rid of it for now and will consider re-adding it once I have time to re-architecture the tests for the very picky CI machine :) |
Description
There has been some inconsistent failures with this test recently in the new CI jobs. We had issues with these tests previously which I thought I fixed and could no longer repro locally. This test is still having problems, though, so I am disabling it and will be looking into a new way to test out this scenario that is more reliable and doesn't have a dependency on multiple threads.
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
project.json
andAssemblyInfo.cs
files have been updated with the new version of the SDK.