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 tests because react-netty aggregate random order of response … #7770

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

mssfang
Copy link
Member

@mssfang mssfang commented Jan 28, 2020

Disabled listing tests because of unstable react-netty aggregate random order of responses.

Opened a tracking issue for these disabled tests: #7771

@mssfang mssfang added Client This issue points to a problem in the data-plane of the library. App Configuration Azure.ApplicationModel.Configuration labels Jan 28, 2020
@mssfang mssfang requested review from joshfree and anuchandy January 28, 2020 19:27
@mssfang mssfang self-assigned this Jan 28, 2020
@alzimmermsft
Copy link
Member

Not a fan of disabling failing tests due to a legitimate issue in the SDK/underlying infrastructure, this feels like a scenario where we say we'll get around to resolving the underlying issue soon but forget until we get a customer reported issue.

If we end up merging this please submit an issue for the underlying problem and get it triaged appropriately so we don't let the problem fester.

@JonathanGiles
Copy link
Member

I agree with @alzimmermsft. I do not want to be in the business of disabling tests eagerly. I would like to understand more about what the issue is and why we can't fix the underlying issue, before we disable these tests.

@mssfang
Copy link
Member Author

mssfang commented Jan 28, 2020

@anuchandy and I had investigated the failure of tests. I am also not a fan of disabling tests. But since it caused by the react-netty aggregate response content in random order, we can't really solve this problem immediately and we want to make sure other tests work property. Before we file an issue to react-netty, Anu is investigating it and double confirm.

@JonathanGiles
Copy link
Member

Have these tests ever successfully passed in successive runs, or have they always been flakey?

Copy link
Member

@joshfree joshfree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put the GitHub Issue # in the disable comment.

Also please open a bug on react-netty and include a link to it in the tracking-external-issue issue.

@mssfang
Copy link
Member Author

mssfang commented Jan 28, 2020

@JonathanGiles These tests some times failed some times successfully passed.

@JonathanGiles
Copy link
Member

@mssfang To avoid any confusion, this PR is now pending you making the changes that Josh has requested.

@mssfang
Copy link
Member Author

mssfang commented Jan 29, 2020

@JonathanGiles Yes. Anu wants to have a final verified before file an issue. Will file an issue and merged it after confirmed.

@mssfang
Copy link
Member Author

mssfang commented Jan 29, 2020

So we are going to merge this PR. Ran a live test pipeline on this PR: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=246641&view=results. All other tests look good. We are keeping investigating it tomorrow. And leave the issue #7771 open.

@mssfang mssfang merged commit feb021d into Azure:master Jan 29, 2020
@mssfang mssfang deleted the LiveTestAzConfig branch January 29, 2020 03:18
@anuchandy
Copy link
Member

Related PR with fix: #7837

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants