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.

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

Merged
merged 4 commits into from
Apr 28, 2017
Merged

Disable a flaky ADLS test for now #3135

merged 4 commits into from
Apr 28, 2017

Conversation

begoldsm
Copy link
Contributor

@begoldsm begoldsm commented Apr 27, 2017

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

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as a link to the swagger spec, used to generate the code.
  • The project.json and AssemblyInfo.cs files have been updated with the new version of the SDK.

begoldsm added 3 commits April 27, 2017 10:28
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")]
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.
@begoldsm
Copy link
Contributor Author

@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 :)

@markcowl markcowl merged commit c67fdf6 into Azure:vs17Dev Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants