chore: use `@web/test-runner` instead of `karma` #1816

SimenB · 2023-11-20T08:51:50Z

Which problem is this PR solving?

Attempting to unblock feat: bring back browser extension #1152 - i.e. bringing back the browser extension
Note that this just affects @opentelemetry/instrumentation-document-load for now - I wanna get some feedback from the team that that this is something you want before spending more time on it 🙂

Short description of the changes

As mentioned in feat: bring back browser extension #1152 (comment), karma is deprecated, and they recommend using Jest or @web/test-runner as a replacement. Seeing as I guess these theses should run in the browser, I elected to go with @web/test-runner
Under the hood wtr already uses mocha, so that migration is free. However, we also need to replace webpack with rollup/esbuild - that requires some more changes. I was unable to get assert to work, so I replaced it with chai.
Coverage is collected, but I'm unsure if CI will correctly pick it up (EDIT: See later comments - the coverage is picked up, but something's weird with the merged report on codecov)

SimenB · 2023-11-20T09:30:13Z

plugins/web/opentelemetry-instrumentation-document-load/test/documentLoad.test.ts

-          'http://localhost:9876/context.html'
+        assert.isOk(
+          (fetchSpan.attributes['http.url'] as string).startsWith(
+            'http://localhost:8000/?wtr-session-id='


URL is different, and the session ID unique per run. I think this is fine, but happy to change to whatever you want 🙂

SimenB · 2023-11-20T09:30:36Z

plugins/web/opentelemetry-instrumentation-document-load/test/documentLoad.test.ts

+// @ts-expect-error: not an export, but we want the prebundled version
+import chai from 'chai/chai.js';


ideally we'd just get assert to work in the browser, but for some reason it gives an error

We used the webpack node polyfills for assert (see

opentelemetry-js-contrib/webpack.node-polyfills.js

Lines 3 to 6 in 434cab2

assert: true,

// The assert polyfill from github.com/browserify/commonjs-assert

// also requires the `global` polyfill.

global: true,

).

IMO not using node polyfills for tests that are only intended for the browser is a good idea - I'd be fine for this to switch to chai, would like to get an opinion on it by @martinkuba or @pkanal first though as they're the subject matter experts 🙂

codecov · 2023-11-20T09:39:22Z

Codecov Report

Merging #1816 (0b71795) into main (dfb2dff) will decrease coverage by 0.13%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1816      +/-   ##
==========================================
- Coverage   90.97%   90.85%   -0.13%     
==========================================
  Files         146      143       -3     
  Lines        7492     7368     -124     
  Branches     1502     1468      -34     
==========================================
- Hits         6816     6694     -122     
+ Misses        676      674       -2

see 4 files with indirect coverage changes

SimenB · 2023-11-20T09:46:43Z

Hmm, we need to upgrade the node version used to run the browser tests. Not sure how to best do that in a way that still works with the karma ones? or maybe karma works on latest release

SimenB · 2023-11-20T10:09:31Z

OK, using Node 16 works with both old webpack and the WTR runner 👍

SimenB · 2023-11-20T11:25:22Z

Hmm, unsure why code coverage was lost - the output file was located: https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/6928406995/job/18844148357?pr=1816#step:8:79

Pushed an attempt to also produce json output - let's see 😀

This reverts commit af20f6c.

SimenB · 2023-11-20T11:48:48Z

Wait, is it correct? Codecov UI is confusing me 😅

But it is covered if I open coverage/lcov-report/index.html

No clue 😅

This reverts commit 6e3b6fe.

pichlermarc · 2023-11-20T12:18:17Z

Wait, is it correct? Codecov UI is confusing me 😅

Looks like these are the indirect changes, as far as I understand (codecov is confusing me too 😅) that's changes in coverage to files that have themselves not changed in the PR.

This link should be the one to look at (when Unit Tests / browser-test (16) (pull_request) check is green, the latest reports are uploaded and it'll accurately reflect what's there) - if instrumentation.ts is in src/ and has some coverage reported for it then it's working. 🙂

That being said, the codecov upload in this repo is sometimes flaky (we (the repo maintainers) may have to add a token for it to work properly, though no idea why other runs work fine without it, EDIT: it may be related to this). In the current run it did not upload because of an error, but that error is not related to your PR. The file is actually here, https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/6929736595/job/18848043256?pr=1816#step:8:69 so coverage seems to be working fine 🙂 - it's just codecov that's not uploading it.

This reverts commit 11100a8.

SimenB · 2023-11-20T12:27:27Z

At least running npm run test:browser && open coverage/lcov-report/index.html locally shows correct coverage information, so I think the test runner changes are correct. Getting codecov to agree shouldn't be that hard afterwards if it's some sort of permission/from fork weirdness that makes the codecov app report other coverage data.

dyladan · 2023-11-20T13:53:18Z

Under the hood wtr already uses mocha, so that migration is free. However, we also need to replace webpack with rollup/esbuild - that requires some more changes. I was unable to get assert to work, so I replaced it with chai.

Removing webpack is something we've wanted to do anyway. In the core repo we've been working on updating tooling that was held back by the necessity to support old node versions which are being dropped soon.

Coverage is collected, but I'm unsure if CI will correctly pick it up (EDIT: See later comments - the coverage is picked up, but something's weird with the merged report on codecov)

Coverage has been in a weird state in this repo for a while. It is likely not your fault.

SimenB · 2023-11-20T14:25:06Z

Cool! In that case, should I remove karma from the other packages as well so we don't have to run both systems?

This PR can also be merged on its own, and then I can follow up for the other packages. Whatever's most convenient to you 👍

svrnm

This will unblock the browser extension:-) , thank you @SimenB

pichlermarc · 2023-11-21T13:44:01Z

Cool! In that case, should I remove karma from the other packages as well so we don't have to run both systems?

This PR can also be merged on its own, and then I can follow up for the other packages. Whatever's most convenient to you 👍

I think having follow ups to adapt the other instrumentations would be appreciated (once this PR has merged). We'd like to keep tooling in sync as much as possible. 🙂

SimenB · 2023-11-21T14:56:58Z

Yeah, happy to do follow-ups - should be quick. I was more asking if you want one big bang in a single PR or split up.

pichlermarc · 2023-11-21T15:06:17Z

Yeah, happy to do follow-ups - should be quick. I was more asking if you want one big bang in a single PR or split up.

Amazing, thx! I'd prefer having it split up :)

SimenB · 2023-11-21T15:56:55Z

Sweet 👍 will do once this is merged

pichlermarc · 2023-11-27T09:43:37Z

cc @martinkuba @pkanal (component-owners) - any objections? 🙂
If not then I'd merge this in. 🙂

pichlermarc

Brought this up in the SIG Meeting this week and there were no objections to switching. 🙂

pichlermarc · 2024-03-06T15:41:22Z

@SimenB would you mind giving that PR an update? 🙂 I had been planning to merge it in a while ago but due to conflicts merging was blocked.

SimenB · 2024-03-08T12:57:15Z

hopefully CI is happy 🙂

SimenB · 2024-03-08T12:59:48Z

looks like it 👍

pichlermarc · 2024-03-08T14:18:44Z

awesome, thanks 🙂

chore: use @web/test-runner instead of karma

0490976

SimenB requested a review from a team November 20, 2023 08:51

github-actions bot assigned martinkuba and pkanal Nov 20, 2023

github-actions bot requested review from martinkuba and pkanal November 20, 2023 08:52

SimenB added 3 commits November 20, 2023 09:53

remove karma config file

d5d32bd

prettier

e8de8f7

ts

25ae373

pichlermarc added the pkg:instrumentation-document-load label Nov 20, 2023

more ts

438b2f2

SimenB commented Nov 20, 2023

View reviewed changes

remove one more dep

b319014

maybe run browser test with node 18

3412058

pichlermarc mentioned this pull request Nov 20, 2023

[next] Deprecate old runtimes open-telemetry/opentelemetry-js#4295

Merged

we need 16

45f70e7

is json output enough?

af20f6c

SimenB added 2 commits November 20, 2023 12:28

Revert "is json output enough?"

6e3b6fe

This reverts commit af20f6c.

Merge branch 'main' into replace-karma

541071c

Revert "Revert "is json output enough?""

11100a8

This reverts commit 6e3b6fe.

Revert "Revert "Revert "is json output enough?"""

1f46a5f

This reverts commit 11100a8.

trigger ci

07408ec

svrnm approved these changes Nov 21, 2023

View reviewed changes

pichlermarc added 2 commits January 5, 2024 11:58

Merge branch 'main' into replace-karma

c566d25

chore: sync package-lock.json

b8c1d53

pichlermarc approved these changes Jan 5, 2024

View reviewed changes

SimenB added 2 commits March 8, 2024 13:51

Merge branch 'main' into replace-karma

791bc90

remove two unused deps

0b71795

pichlermarc merged commit 19ebbaf into open-telemetry:main Mar 8, 2024
17 checks passed

SimenB deleted the replace-karma branch March 8, 2024 14:32

SimenB mentioned this pull request Mar 8, 2024

chore: replace karma with wpt runner in user-interaction #2005

Open

JamieDanielson mentioned this pull request Mar 26, 2024

refactor(instr-document-load): use exported strings for semconv #2039

Merged

pichlermarc mentioned this pull request Apr 15, 2024

chore(deps): update dependency chai to v5 #2070

Closed

JamieDanielson mentioned this pull request May 23, 2024

feat: bring back browser extension #1152

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use `@web/test-runner` instead of `karma` #1816

chore: use `@web/test-runner` instead of `karma` #1816

SimenB commented Nov 20, 2023 •

edited

Loading

SimenB Nov 20, 2023

SimenB Nov 20, 2023

pichlermarc Nov 21, 2023

codecov bot commented Nov 20, 2023 •

edited

Loading

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

pichlermarc commented Nov 20, 2023 •

edited

Loading

SimenB commented Nov 20, 2023

dyladan commented Nov 20, 2023

SimenB commented Nov 20, 2023 •

edited

Loading

svrnm left a comment

pichlermarc commented Nov 21, 2023

SimenB commented Nov 21, 2023

pichlermarc commented Nov 21, 2023 •

edited

Loading

SimenB commented Nov 21, 2023

pichlermarc commented Nov 27, 2023

pichlermarc left a comment •

edited

Loading

pichlermarc commented Mar 6, 2024

SimenB commented Mar 8, 2024

SimenB commented Mar 8, 2024

pichlermarc commented Mar 8, 2024

		// @ts-expect-error: not an export, but we want the prebundled version
		import chai from 'chai/chai.js';

	assert: true,
	// The assert polyfill from github.com/browserify/commonjs-assert
	// also requires the `global` polyfill.
	global: true,

chore: use @web/test-runner instead of karma #1816

chore: use @web/test-runner instead of karma #1816

Conversation

SimenB commented Nov 20, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

SimenB Nov 20, 2023

Choose a reason for hiding this comment

SimenB Nov 20, 2023

Choose a reason for hiding this comment

pichlermarc Nov 21, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 20, 2023 • edited Loading

Codecov Report

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

SimenB commented Nov 20, 2023

pichlermarc commented Nov 20, 2023 • edited Loading

SimenB commented Nov 20, 2023

dyladan commented Nov 20, 2023

SimenB commented Nov 20, 2023 • edited Loading

svrnm left a comment

Choose a reason for hiding this comment

pichlermarc commented Nov 21, 2023

SimenB commented Nov 21, 2023

pichlermarc commented Nov 21, 2023 • edited Loading

SimenB commented Nov 21, 2023

pichlermarc commented Nov 27, 2023

pichlermarc left a comment • edited Loading

Choose a reason for hiding this comment

pichlermarc commented Mar 6, 2024

SimenB commented Mar 8, 2024

SimenB commented Mar 8, 2024

pichlermarc commented Mar 8, 2024

chore: use `@web/test-runner` instead of `karma` #1816

chore: use `@web/test-runner` instead of `karma` #1816

SimenB commented Nov 20, 2023 •

edited

Loading

codecov bot commented Nov 20, 2023 •

edited

Loading

pichlermarc commented Nov 20, 2023 •

edited

Loading

SimenB commented Nov 20, 2023 •

edited

Loading

pichlermarc commented Nov 21, 2023 •

edited

Loading

pichlermarc left a comment •

edited

Loading