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] Regenerating all the recordings at once fail because of the tests with this.skip() #5697

Closed
HarshaNalluru opened this issue Oct 21, 2019 · 4 comments
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) test-utils-recorder Label for the issues related to the common recorder

Comments

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Oct 21, 2019

image

Record mode - NODE tests.

The tests with this.skip() doesn't execute the recorder.stop() call in the succeeding afterEach(since afterEach itself is not executed) which affects the tests that are run later on to throw an error saying NOCK recording is already in progress.

/cc - @chradek

@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 21, 2019
@HarshaNalluru HarshaNalluru changed the title [Storage] Regenerating all the recordings fail because of the tests with this.skip() [Storage] Regenerating all the recordings at once fail because of the tests with this.skip() Oct 30, 2019
@HarshaNalluru HarshaNalluru self-assigned this Nov 1, 2019
@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Nov 5, 2019

The correct behaviour should be that afterEach block should be executed when a test is skipped just like the beforeEach block, which would ensure that the ongoing recording is stopped with recorder.stop().

This is a bug in the mocha library that afterEach block is not being executed if a test is skipped unlike beforeEach.

This hasn't been fixed in 6.2.2 version(latest).
Version 7.0.0(hasn't been released yet) will have the fix.

Update - Tested this branch mochajs/mocha#3741, this seems to fix things :)

@HarshaNalluru
Copy link
Member Author

The issue is fixed in mocha 7.0.0 - mochajs/mocha#3741

However, after updating mocha to 7.0.0 in the central repo's SDKs,
rush update failed as below.

image

This issue occurred because mocha-multi is still relying on an older version of mocha.

For comparison, npm throws a warning only
image

glenjamin/mocha-multi#68 - Logged an issue in the mocha-multi repo asking them to support mocha@7.0.0

@ramya-rao-a
Copy link
Contributor

@HarshaNalluru Is there anything we need to do for Storage specifically here if we update mocha across the repo?

@HarshaNalluru
Copy link
Member Author

This was seen with storage at the time. It should be independent of storage.
We logged issues, I had verified their PR upon their request for the bug fix.
And they eventually released the new version.
And long back, I had added an alternative for mocha-multi, so nothing left to do here.

@xirzec xirzec removed this from the Backlog milestone May 18, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) test-utils-recorder Label for the issues related to the common recorder
Projects
None yet
Development

No branches or pull requests

3 participants