feat(cucumber): support @cucumber/cucumber@10 #1830

Ugzuzg · 2023-11-27T15:55:00Z

Which problem is this PR solving?

The new version of @cucumber/cucumber is out.

Short description of the changes

Bump the supported version in the instrumentation.

trentm

The TAV test failed for all tested node versions (14, 16, 18) on the same thing:

...
@opentelemetry/instrumentation-cucumber: -- installing ["@cucumber/cucumber@9.1.2"]
@opentelemetry/instrumentation-cucumber: -- running test "npm test" with @cucumber/cucumber (env: {})
@opentelemetry/instrumentation-cucumber: > @opentelemetry/instrumentation-cucumber@0.1.2 test
@opentelemetry/instrumentation-cucumber: > nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'
@opentelemetry/instrumentation-cucumber: Error: Cannot find module '@cucumber/messages'
@opentelemetry/instrumentation-cucumber: Require stack:
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@cucumber/gherkin-streams/dist/src/makeGherkinOptions.js
...

I wonder if cucumber v9.1.2 was a bad release? If so -- I haven't looked into it -- then we could skip that release in the .tav.yml file.

trentm · 2023-11-28T16:02:19Z

plugins/node/instrumentation-cucumber/.tav.yml

@@ -1,3 +1,3 @@
 '@cucumber/cucumber':
-  versions: '^8.0.0 || ^9.0.0'
+  versions: '^8.0.0 || ^9.0.0 || ^10.0.0'


Although the part of the TAV (test-all-versions) tests that tested cucumber v10 did pass, technically v10 dropped support for Node.js v14 and v16 -- https://github.com/cucumber/cucumber-js/releases/tag/v10.0.0 -- so I wonder if we may want a .tav.yml file like this:

'@cucumber/cucumber': - versions: '^8.0.0 || ^9.0.0' node: ">=14 <20" commands: npm test - versions: '^10.0.0' node: ">=18" commands: npm test

trentm · 2023-11-28T16:03:48Z

plugins/node/instrumentation-cucumber/package.json

@@ -43,7 +43,7 @@
    "@opentelemetry/api": "^1.0.0"
  },
  "devDependencies": {
-    "@cucumber/cucumber": "^9.0.0",
+    "@cucumber/cucumber": "^10.0.0",


Given cucumber v10 dropped Node.js v14 and v16 support that this package still supports, I think we want to keep the version here on cucumber v9. Currently @cucumber/cucumber@10.0.1 seems to work with tests with Node.js v14 and v16, but that could break later.

codecov · 2023-11-28T17:29:32Z

Codecov Report

Merging #1830 (94d25c6) into main (4ca1862) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1830   +/-   ##
=======================================
  Coverage   91.49%   91.49%           
=======================================
  Files         144      144           
  Lines        7388     7388           
  Branches     1474     1474           
=======================================
  Hits         6760     6760           
  Misses        628      628

Files	Coverage Δ
...de/instrumentation-cucumber/src/instrumentation.ts	`92.30% <ø> (ø)`

trentm · 2023-11-28T17:55:28Z

@Ugzuzg I don't know that is it a policy for this repo or anything, but IMHO it would be helpful if you did not force push updates to a PR branch after having gotten some reviews. It means that I cannot go see exactly what the state was for earlier review comments. For example, I'm looking at the package-lock.json changes now and I'm curious why the @cucumber/cucumber dep moved from the top-level node_modules folder to the plugins/node/instrumentation-cucumber/node_modlues folder. Not a big deal at all, however. Thanks for the PR!

trentm · 2023-11-28T19:18:44Z

package-lock.json

Could you remove the package-lock.json change from the PR? While the changes in here are fine, I'm guessing the move of @cucumber/cucumber and some of its deps from "./node_modules" to "./plugins/node/instrumentation-cucumber/node_modules/..." is a side-effect of having updated to v10 and then backed up to v9. package-lock.json handling, especially with npm workspaces, is a bit of voodoo.

I think this change is good to merge either way (without or with the package-lock.json change).

dyladan · 2023-11-28T19:29:39Z

@Ugzuzg I don't know that is it a policy for this repo or anything, but IMHO it would be helpful if you did not force push updates to a PR branch after having gotten some reviews. It means that I cannot go see exactly what the state was for earlier review comments.

Not a specific policy for this repo, but I agree changing history makes reviews a lot harder. We can't enforce a policy because changes are happening on forks.

trentm · 2023-11-28T22:01:14Z

TAV tests are failing on main with the same failure that is happening here:

@opentelemetry/instrumentation-cucumber: -- installing ["@cucumber/cucumber@9.1.2"]
@opentelemetry/instrumentation-cucumber: -- running test "npm test" with @cucumber/cucumber (env: {})
@opentelemetry/instrumentation-cucumber: > @opentelemetry/instrumentation-cucumber@0.1.2 test
@opentelemetry/instrumentation-cucumber: > nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'
@opentelemetry/instrumentation-cucumber: Error: Cannot find module '@cucumber/messages'
@opentelemetry/instrumentation-cucumber: Require stack:
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@cucumber/gherkin-streams/dist/src/makeGherkinOptions.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@cucumber/gherkin-streams/dist/src/GherkinStreams.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/@cucumber/gherkin-streams/dist/src/index.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-cucumber/node_modules/@cucumber/cucumber/lib/api/gherkin.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-cucumber/node_modules/@cucumber/cucumber/lib/api/load_sources.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-cucumber/node_modules/@cucumber/cucumber/lib/api/index.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/plugins/node/instrumentation-cucumber/test/cucumber.test.ts
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/mocha/lib/esm-utils.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/mocha/lib/mocha.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/mocha/lib/cli/one-and-dones.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/mocha/lib/cli/options.js
@opentelemetry/instrumentation-cucumber: - /home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/mocha/bin/mocha
@opentelemetry/instrumentation-cucumber:     at Function.Module._resolveFilename (internal/modules/cjs/loader.js:931:15)
@opentelemetry/instrumentation-cucumber:     at Function.Module._load (internal/modules/cjs/loader.js:774:27)

So now I wonder if that odd package-lock.json addition in commit 3a8e7cf is helpful.

The Error: Cannot find module '@cucumber/messages' smells like a npm workspaces issue: @cucumber/cucumber installed in one node_modules tree cannot find its dep @cucumber/messages over in some other node_modules tree. However, I'm not sure. I'm trying to repro locally to debug.

trentm · 2023-11-29T18:09:59Z

I'm trying to repro locally to debug.

I'll move discussion on this to #1828

@Ugzuzg Do you happen to know how you got the package-lock.json change that you did? Was it intentional, or was it a side-effect of the npm install ... commands that you did to bump cucumber to v10 and then back to v9? The reason I'm asking is because I've been unable to fully understand the Error: Cannot find module '@cucumber/messages' test failure above, so I'm gathering any possibly helpful information.

Ugzuzg · 2023-11-29T18:13:55Z

Yes, it was just an npm install.

trentm · 2023-12-01T19:34:57Z

TAV tests will fail until #1838 is merged, but I think this is good to go in now.

Ugzuzg requested a review from a team November 27, 2023 15:55

github-actions bot assigned Ugzuzg Nov 27, 2023

Ugzuzg force-pushed the feat/cucumber-bump branch from c9c44dd to 9c77114 Compare November 27, 2023 15:58

david-luna approved these changes Nov 28, 2023

View reviewed changes

david-luna mentioned this pull request Nov 28, 2023

chore(deps): update dependency @cucumber/cucumber to v10 #1734

Closed

1 task

pichlermarc added the pkg:instrumentation-cucumber label Nov 28, 2023

trentm requested changes Nov 28, 2023

View reviewed changes

Ugzuzg added 3 commits November 28, 2023 18:31

feat(cucumber): support @cucumber/cucumber@10

b36e5fa

chore(cucumber): configure tav versions

f830047

chore(cucumber): downgrade dev dependency to 9

3a8e7cf

Ugzuzg force-pushed the feat/cucumber-bump branch from 3c20801 to 3a8e7cf Compare November 28, 2023 17:31

trentm approved these changes Nov 28, 2023

View reviewed changes

Revert package lock changes

85c36e7

dyladan approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'main' into feat/cucumber-bump

01ff381

Merge branch 'main' into feat/cucumber-bump

94d25c6

trentm merged commit 1c2e8b2 into open-telemetry:main Dec 1, 2023
12 of 15 checks passed

dyladan mentioned this pull request Dec 1, 2023

chore: release main #1829

Merged

Ugzuzg mentioned this pull request Jan 16, 2025

REQUEST: New membership for Ugzuzg open-telemetry/community#2517

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cucumber): support @cucumber/cucumber@10 #1830

feat(cucumber): support @cucumber/cucumber@10 #1830

Ugzuzg commented Nov 27, 2023 •

edited

Loading

trentm left a comment

trentm Nov 28, 2023

trentm Nov 28, 2023

codecov bot commented Nov 28, 2023 •

edited

Loading

trentm commented Nov 28, 2023

trentm Nov 28, 2023

dyladan commented Nov 28, 2023

trentm commented Nov 28, 2023

trentm commented Nov 29, 2023

Ugzuzg commented Nov 29, 2023

trentm commented Dec 1, 2023

feat(cucumber): support @cucumber/cucumber@10 #1830

feat(cucumber): support @cucumber/cucumber@10 #1830

Conversation

Ugzuzg commented Nov 27, 2023 • edited Loading

Which problem is this PR solving?

Short description of the changes

trentm left a comment

Choose a reason for hiding this comment

trentm Nov 28, 2023

Choose a reason for hiding this comment

trentm Nov 28, 2023

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2023 • edited Loading

Codecov Report

trentm commented Nov 28, 2023

trentm Nov 28, 2023

Choose a reason for hiding this comment

dyladan commented Nov 28, 2023

trentm commented Nov 28, 2023

trentm commented Nov 29, 2023

Ugzuzg commented Nov 29, 2023

trentm commented Dec 1, 2023

Ugzuzg commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 28, 2023 •

edited

Loading