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

[Core] - Update readme, fix test paths, and remove OpenCensus wrappers #15770

Merged
merged 8 commits into from Jun 16, 2021

Conversation

maorleger
Copy link
Member

What

  • Add missing headers to Core Tracing README
  • Remove OpenCensus Wrappers
  • Fix dist paths
  • Enable browser tests

Why

  • Ensures that our package README conforms to guidelines so we can remove the exception for core-tracing
  • My understanding is that OpenTelemetry was compatible with OpenCensus but has since drifted far enough that direct mappings are no longer possible.

The other two I noticed in passing - when running tests after switching branches I saw old test code run that no longer exists.
When I looked into it, I noticed that we were still generating into test-dist but changed our commands to expect dist-test.

This led me down a little rabbit hole of looking at our rollup configs so I decided to just go ahead and clean this up wholesale -
adding browser tests, fixing all the paths, etc.

Resolves #15732
Resolves #14725

// https://github.com/karma-runner/karma-chrome-launcher
process.env.CHROME_BIN = require("puppeteer").executablePath();

module.exports = function(config) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't bother too much, or at all, with karma.conf.js. I just took what is in core-rest-client as is.

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.

I love this change! ❤️

Noticed some of the type exports seem to be lingering, not sure if you just need to regenerate the api.md or what.

sdk/core/core-tracing/review/core-tracing.api.md Outdated Show resolved Hide resolved
Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

This looks like a good change to me.

Removing stuff warrants a mention in the changelog. I don't know if we have suggested migration steps for people there (like - write their own wrapper/adapter?).

"strictEqual",
"notDeepEqual",
"notStrictEqual",
"notDeepStrictEqual"
Copy link
Member

Choose a reason for hiding this comment

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

Ha, you add one more item to the list and it blows up the formatting..

@maorleger maorleger merged commit 713d433 into Azure:master Jun 16, 2021
@maorleger maorleger deleted the lightweight-items branch June 16, 2021 21:50
sadasant pushed a commit to sadasant/azure-sdk-for-js that referenced this pull request Jun 21, 2021
Azure#15770)

## What

- Add missing headers to Core Tracing README
- Remove OpenCensus Wrappers
- Fix dist paths
- Enable browser tests

## Why

- Ensures that our package README conforms to guidelines so we can remove the exception for core-tracing
- My understanding is that OpenTelemetry was compatible with OpenCensus but has since drifted far enough that direct mappings are no longer possible. 

The other two I noticed in passing - when running tests after switching branches I saw old test code run that no longer exists.
When I looked into it, I noticed that we were still generating into `test-dist` but changed our commands to expect `dist-test`. 

This led me down a little rabbit hole of looking at our rollup configs so I decided to just go ahead and clean this up wholesale - 
adding browser tests, fixing all the paths, etc.

Resolves Azure#15732
Resolves Azure#14725
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core-tracing] Remove OpenCensus wrappers [core core-tracing] Missing readme headers
3 participants