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

[Storage] Remove retries for the tests #6910

Merged
merged 2 commits into from
Jan 16, 2020

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Jan 10, 2020

Removing mocha retry configs for both the node tests and the browser tests
(Meaning, if a test fails, it won't be re-run)

@ramya-rao-a
Copy link
Contributor

Any links to prior discussions on why we are removing the retries?

@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jan 13, 2020

More Info

#4196

In the current retry mechanism for the tests, we have enabled mocha retries at the test-suite level which might hide flaky tests and we might potentially lose any hidden bugs with the SDK or even with the service.
In case we are addressing the flaky connectivity issues by retrying, the ideal solution is to be scoping it better(like retrying that specific request).

@mikeharder is currently working on getting the live tests to succeed at least 90% without the retries for tests.

@HarshaNalluru HarshaNalluru marked this pull request as ready for review January 15, 2020 04:28
Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Just a question, but why are retries and timeout specified in both karma.conf.js and the mocha command-line? When is karma.conf.js used?

This PR is a good start, but it should not be merged until we are fairly certain it won't reduce the pass rate. We should run each impacted live test pipeline a few times (via /azp run comments), fix any new failures (in separate PRs), and keep iterating until the pass rate is the same as master. If some pipelines are ready before others, we can split this PR into one per pipeline.

@mikeharder
Copy link
Member

/azp run js - storage-blob - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-file-share - tests

@mikeharder
Copy link
Member

/azp run js - storage-queue - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

@HarshaNalluru: Does storage-file-datalake have live tests? I don't see a pipeline with this name.

@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jan 15, 2020

but why are retries and timeout specified in both karma.conf.js and the mocha command-line? When is karma.conf.js used?

  • Node tests rely on the integration-test:node command, which is where we set the mocha retries for the node tests.
  • Browser tests rely on integration-test:browser command(karma runs the browser tests for us), which relies on karma.conf.js, which is where we set the retries for the browser tests.

This PR is a good start, but it should not be merged until we are fairly certain it won't reduce the pass rate.

Sounds good.

@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jan 15, 2020

Does storage-file-datalake have live tests? I don't see a pipeline with this name.

It is a recently added package, it doesn't have the live tests pipeline, I don't think we are tracking it with an issue. /cc - @jeremymeng

@jeremymeng
Copy link
Member

It is a recently added package, it doesn't have the live tests pipeline

@XiaoningLiu has been working with EngSys team to set up the pipeline.

@mikeharder
Copy link
Member

All pipelines passed on the first run. I am somewhat surprised the retries are no longer needed, but it seems to be the case. If they pass one more time this should be merged.

@mikeharder
Copy link
Member

/azp run js - storage-blob - tests

@mikeharder
Copy link
Member

/azp run js - storage-file-share - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-queue - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-queue - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-blob - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-queue - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

/azp run js - storage-file-share - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mikeharder
Copy link
Member

Based on the test runs, this may introduce a few new intermittent failures, but this seems fine since we are actively investigating and fixing them. Ready to merge.

@HarshaNalluru HarshaNalluru merged commit 53b4f08 into Azure:master Jan 16, 2020
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.

4 participants