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

chore: upgrade to mocha@10 #2481

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Oct 11, 2024

From mocha@7. Drop use of ts-mocha in favour of a .mocharc.yml.
This mimicks the same mocha@10 update that was done in opentelemetry-js.git
a long while back.

Refs: #1826 (comment)

From mocha@7. Drop use of ts-mocha in favour of a .mocharc.yml.
This mimicks the same mocha@10 update that was done in opentelemetry-js.git
a long while back.

Refs: open-telemetry#1826 (comment)
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.49%. Comparing base (b043ffb) to head (c993c34).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2481      +/-   ##
==========================================
- Coverage   90.74%   90.49%   -0.26%     
==========================================
  Files         156      151       -5     
  Lines        7728     7418     -310     
  Branches     1590     1550      -40     
==========================================
- Hits         7013     6713     -300     
+ Misses        715      705      -10     

see 5 files with indirect coverage changes

@trentm
Copy link
Contributor Author

trentm commented Oct 11, 2024

npm test in instr-router is failing:

 > @opentelemetry/[email protected] test
> nyc mocha 'test/**/*.ts'


 Exception during run: test/index.test.ts(30,25): error TS7016: Could not find a declaration file for module 'router'. '/home/runner/work/opentelemetry-js-contrib/opentelemetry-js-contrib/node_modules/router/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/router` if it exists or add a new declaration (.d.ts) file containing `declare module 'router';`
Error: test/index.test.ts(39,15): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(39,20): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(39,25): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(44,23): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(44,28): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(44,33): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(48,38): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(48,43): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(48,48): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(54,43): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(54,48): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(54,53): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(63,51): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(63,56): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(63,61): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(74,51): error TS7006: Parameter 'err' implicitly has an 'any' type.
Error: test/index.test.ts(74,56): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(74,61): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(74,66): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(79,38): error TS7006: Parameter 'req' implicitly has an 'any' type.
Error: test/index.test.ts(79,43): error TS7006: Parameter 'res' implicitly has an 'any' type.
Error: test/index.test.ts(79,48): error TS7006: Parameter 'next' implicitly has an 'any' type.
Error: test/index.test.ts(88,22): error TS7006: Parameter 'err' implicitly has an 'any' type.

I don't know how to solve this. @david-luna If you have a chance to look at this and provide TypeScript expertise, I'd appreciate it.

Copy link
Contributor

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature.
Are you familiar with this package? Consider becoming a component owner.

@david-luna
Copy link
Contributor

@trentm

for some reason with this configuration the tests are not getting the router types from ./src/router.d.ts. Probably is ts-node making a slight different tsconfig.json

cd ./plugins/node/opentelemetry-instrumentation-router

npmx tsc --showConfig

{
    "compilerOptions": {
        "allowUnreachableCode": false,
        "allowUnusedLabels": false,
        "declaration": true,
        "declarationMap": true,
        "forceConsistentCasingInFileNames": true,
        "inlineSources": true,
        "module": "commonjs",
        "noEmitOnError": true,
        "noFallthroughCasesInSwitch": true,
        "noImplicitOverride": true,
        "noImplicitReturns": true,
        "noUnusedLocals": true,
        "sourceMap": true,
        "strict": true,
        "strictNullChecks": true,
        "target": "es2017",
        "incremental": true,
        "newLine": "lf",
        "rootDir": "./",
        "outDir": "./build"
    },
    "files": [
        "./src/constants.ts",
        "./src/index.ts",
        "./src/instrumentation.ts",
        "./src/internal-types.ts",
        "./src/router.d.ts",  <-- router types are included
        "./src/utils.ts",
        "./src/version.ts",
        "./src/enums/AttributeNames.ts",
        "./src/enums/LayerType.ts",
        "./test/index.test.ts"
    ],
    "include": [
        "src/**/*.ts",
        "test/**/*.ts"
    ],
    "exclude": [
        "../../../node_modules"
    ]
}

npx ts-node --showConfig

{
  "ts-node": {
    "cwd": "/Users/david/Documents/repos/otel/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-router",
    "projectSearchDir": "/Users/david/Documents/repos/otel/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-router",
    "project": "/Users/david/Documents/repos/otel/opentelemetry-js-contrib/plugins/node/opentelemetry-instrumentation-router/tsconfig.json"
  },
  "compilerOptions": {
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "declaration": false,
    "forceConsistentCasingInFileNames": true,
    "inlineSources": true,
    "module": "commonjs",
    "noEmitOnError": true,
    "noFallthroughCasesInSwitch": true,
    "noImplicitOverride": true,
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "sourceMap": true,
    "strict": true,
    "strictNullChecks": true,
    "target": "es2017",
    "incremental": true,
    "newLine": "lf",
    "rootDir": "./",
    "outDir": "./.ts-node",
    "inlineSourceMap": false,
    "noEmit": false
  }  <-- no "files" defined in this configuration
}

The easies way to have the type definitions in the test is to use triple slash directive pointing to the router.d.ts file.

// file: index.test.ts
/*
 * Copyright The OpenTelemetry Authors
 * ...
 */

/// <reference path="../src/router.d.ts" />

import { context, trace } from '@opentelemetry/api';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';

@trentm trentm added the has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions label Oct 15, 2024
@trentm
Copy link
Contributor Author

trentm commented Oct 15, 2024

/me sponsoring this

@trentm
Copy link
Contributor Author

trentm commented Oct 15, 2024

The easies way to have the type definitions in the test is to use triple slash directive pointing to the router.d.ts file.

Thanks! I'll try that.

@trentm
Copy link
Contributor Author

trentm commented Oct 15, 2024

The easies way to have the type definitions in the test is to use triple slash directive pointing to the router.d.ts file.

Thanks! I'll try that.

I guess this isn't the easiest way, because our lint rules say using the triple-slash-directive is an error. :) Sigh.

@trentm
Copy link
Contributor Author

trentm commented Oct 15, 2024

FWIW a similar test failure happens with packages/opentelemetry-host-metrics. These two packages (host-metrics and instrumentation-router) are the two packages that have .d.ts files committed.

A theory is that the issue relates to require: 'ts-node/register' in .mocharc.yml being insufficient. Perhaps it doesn't find and load the "tsconfig.json" file in each package?

I'd welcome suggestions on what to do here.

…ost-metrics package; use eslint disable comment to avoid complaint about using those triple-slash-refs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has:sponsor This package or feature has a sponsor that has volunteered to review PRs and respond to questions pkg:auto-configuration-propagators pkg:auto-instrumentations-node pkg:auto-instrumentations-web pkg:host-metrics pkg:id-generator-aws-xray pkg:instrumentation-amqplib pkg:instrumentation-aws-lambda pkg:instrumentation-aws-sdk pkg:instrumentation-bunyan pkg:instrumentation-cassandra-driver pkg:instrumentation-connect pkg:instrumentation-cucumber pkg:instrumentation-dataloader pkg:instrumentation-dns pkg:instrumentation-express pkg:instrumentation-fastify pkg:instrumentation-fs pkg:instrumentation-generic-pool pkg:instrumentation-graphql pkg:instrumentation-hapi pkg:instrumentation-ioredis pkg:instrumentation-knex pkg:instrumentation-koa pkg:instrumentation-long-task pkg:instrumentation-lru-memoizer pkg:instrumentation-memcached pkg:instrumentation-mongodb pkg:instrumentation-mongoose pkg:instrumentation-mysql pkg:instrumentation-mysql2 pkg:instrumentation-nestjs-core pkg:instrumentation-net pkg:instrumentation-pg pkg:instrumentation-pino pkg:instrumentation-redis pkg:instrumentation-redis-4 pkg:instrumentation-restify pkg:instrumentation-router pkg:instrumentation-runtime-node pkg:instrumentation-socket.io pkg:instrumentation-tedious pkg:instrumentation-undici pkg:instrumentation-user-interaction pkg:instrumentation-winston pkg:plugin-react-load pkg:propagation-utils pkg:propagator-instana pkg:propagator-ot-trace pkg:redis-common pkg:resource-detector-alibaba-cloud pkg:resource-detector-aws pkg:resource-detector-azure pkg:resource-detector-container pkg:resource-detector-gcp pkg:resource-detector-github pkg:resource-detector-instana pkg:sql-common pkg-status:unmaintained:autoclose-scheduled pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants