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.

Sign up for GitHub

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

Jump to bottom

Separating nyc from the integration tests for Node #6430

Merged
merged 1 commit into from
Dec 18, 2019
Merged

Separating nyc from the integration tests for Node #6430

merged 1 commit into from
Dec 18, 2019

Conversation

sadasant
Copy link
Contributor

@sadasant sadasant commented Dec 5, 2019 •
edited
Loading

nyc is obscuring Mocha's stack traces. It's unable to follow through the references. This is the fastest fix (probably by days of separation from the next fastest fix), and this will incidentally make our Node tests faster.

image

Fixes #6298

@sadasant
[Recorder] Separating nyc from the integration tests for Node
a4527c3 
nyc is obscuring Mocha's stack traces. It's unable to follow through the
references. This is the fastest fix, and this will incidentally make our
Node tests faster.

Fixes Azure#6298
@sadasant sadasant self-assigned this Dec 5, 2019
xirzec
xirzec approved these changes Dec 5, 2019
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Seems like a good change since we're not using coverage data at the moment.

@HarshaNalluru HarshaNalluru mentioned this pull request Dec 5, 2019
Closed
62 tasks
@ramya-rao-a
Copy link
Contributor

I'd prefer not to remove the coverage information from the integration-test script.
Eventually when we do hook up the relevant parts to light up code coverage, we wouldn't want to be in a state to run a separate script nightly that will do repeat everything that the integration-test does.

If the intention here is to help with debugging, then I would suggest we make use of the test script instead. Based on the guidelines (which I can't seem to find at the moment :)), below are the 3 different types of npm scripts we have for testing

  • unit: gets run during PR CI
  • integration: gets run during nightly CI
  • test: is meant for the dev to use. This can be anything that the dev for the library wants it to be.

My suggestion would be to make the test do the same thing as integration minus the nyc part. And then, developers use this to run the test that they see failing in CI and make sense of the trace.

Thoughts?

@xirzec
Copy link
Member

xirzec commented Dec 6, 2019

To me test is the default target run to test the client. unit and integration are merely describing the types of tests being run (e.g. small / mocked vs large / multiple component.)

In past projects we've had a ci test target that does all the tests + coverage. I don't think integration should be ci only, especially since some of our packages only have integration tests and no unit tests.

@xirzec
Copy link
Member

xirzec commented Dec 6, 2019

Said another way, instead of unit and integration you could call these fast and slow. Both should be run to validate a PR/build. Neither require coverage during local runs, both may require coverage on the CI side.

@ramya-rao-a
Copy link
Contributor

cc @bsiegel who had initially come up with the guidelines of what each of the npm scripts were meant to be.

cc @bterlson because, why not? :)

@sadasant
Copy link
Contributor Author

sadasant commented Dec 6, 2019

Thank you all for contributing! The good thing is that there's no rush on merging this PR, so we can wait until we all agree on something.

jeremymeng
jeremymeng reviewed Dec 9, 2019
View reviewed changes
sdk/storage/storage-queue/package.json
@@ -43,7 +43,9 @@
"unit-test:browser": "cross-env TEST_MODE=playback npm run integration-test:browser",
"unit-test:node": "cross-env TEST_MODE=playback npm run integration-test:node",
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"emulator-tests": "cross-env STORAGE_CONNECTION_STRING=UseDevelopmentStorage=true && npm run test:node"
"emulator-tests": "cross-env STORAGE_CONNECTION_STRING=UseDevelopmentStorage=true && npm run test:node",
"coverage-integration-test:node": "nyc npm run integration-test:node",
Copy link
Member

Choose a reason for hiding this comment

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

I assume the code coverage report look similar before and after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code coverage report looks the same as long as, after this change, you do coverage-integration-test:node to get the coverage 🌞

@bsiegel
Copy link
Member

bsiegel commented Dec 10, 2019

The canonical names for the various scripts are documented in the CONTRIBUTING.md here: https://github.com/Azure/azure-sdk-for-js/blob/master/CONTRIBUTING.md#other-npm-scripts. All projects MUST have those scripts defined in their package.json. Guidelines for how the conventional scripts are used by the engineering system are documented in our JavaScript OneNote under Guidelines >
Script Conventions.

@sadasant sadasant mentioned this pull request Dec 13, 2019
Closed
@sadasant
Copy link
Contributor Author

sadasant commented Dec 13, 2019 •
edited
Loading

@bsiegel thank you! We will make sure to address this as part of this issue: #6555 disregard.

HarshaNalluru
HarshaNalluru reviewed Dec 17, 2019
View reviewed changes
sdk/keyvault/keyvault-certificates/package.json
@@ -64,7 +64,9 @@
"test": "npm run build:test && npm run unit-test && npm run integration-test",
"unit-test:browser": "cross-env TEST_MODE=playback npm run integration-test:browser",
"unit-test:node": "cross-env TEST_MODE=playback npm run integration-test:node",
"unit-test": "npm run unit-test:node && npm run unit-test:browser"
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"coverage-integration-test:node": "nyc npm run integration-test:node",
Copy link
Member

@HarshaNalluru HarshaNalluru Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We should take it as a priority and fix the code coverage configs.
Better to have this coverage-integration-test:node step in the CI for nightly tests than integration-test:node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent about two days trying to fix this problem via configuration files and I couldn't do it. I could have missed something. If you find time, try to fix this via Nyc, Istanbul or Mocha configurations, and let me know if you find a way. I would love to go through that approach if possible.

Copy link
Member

Choose a reason for hiding this comment

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

I know, configs are hard, we have issues logged.

@jeremymeng jeremymeng changed the title [Recorder] Separating nyc from the integration tests for Node Separating nyc from the integration tests for Node Dec 18, 2019
@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. MQ-Storage Storage Storage Service (Queues, Blobs, Files) labels Dec 18, 2019
@jeremymeng jeremymeng merged commit 6c8595e into Azure:master Dec 18, 2019
@ramya-rao-a ramya-rao-a mentioned this pull request Dec 19, 2019
Closed
This was referenced Dec 30, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremymeng jeremymeng jeremymeng left review comments

@HarshaNalluru HarshaNalluru HarshaNalluru approved these changes

@xirzec xirzec xirzec approved these changes

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

@jiacfan jiacfan Awaiting requested review from jiacfan

@sophiajt sophiajt Awaiting requested review from sophiajt

@vinjiang vinjiang Awaiting requested review from vinjiang

@XiaoningLiu XiaoningLiu Awaiting requested review from XiaoningLiu

Assignees

@sadasant sadasant

Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Recorder] Ensure that source maps can be used to debug errors during record and playback
6 participants
@sadasant @ramya-rao-a @xirzec @bsiegel @jeremymeng @HarshaNalluru