Alternative custom reporter to mocha-multi #7931
Conversation
ERROR @rush-temp/event-hubs: ts-mocha@6.0.0 requires a peer of mocha@^3.X.X || ^4.X.X || ^5.X.X || ^6.X.X but version 7.1.1 was installed.
HarshaNalluru
changed the title
HarshaNalluru
requested review from
bterlson,
chradek,
daviwil,
jeremymeng,
jiacfan,
sophiajt,
KarishmaGhiya,
ljian3377,
mikeharder,
ramya-rao-a,
richardpark-msft,
sadasant,
schaabs,
southpolesteve,
vinjiang and
willmtemple
as code owners
|
Update to Not upgrading in this PR. Logged #7932 |
| @@ -1,6 +1,5 @@ | |||
| --require ts-node/register | |||
| --timeout 50000 | |||
| --reporter mocha-multi | |||
| --reporter-options spec=-,mocha-junit-reporter=- | |||
| --reporter ../../../common/tools/mocha-multi-reporter.js | |||
There was a problem hiding this comment.
core-arm and core-http packages have mocha.opts file.
TO DO - Remove the mocha.opts file and update test scripts accordingly to stay consistent with the other packages in this repo.
| @@ -32,7 +32,7 @@ | |||
| "test:node": "npm run build:test && npm run unit-test:node && npm run integration-test:node", | |||
| "test": "npm run build:test && npm run unit-test && npm run integration-test", | |||
| "unit-test:browser": "echo skipped", | |||
| "unit-test:node": "mocha test-dist/**/*.js --reporter mocha-multi --reporter-options spec=-,mocha-junit-reporter=-", | |||
| "unit-test:node": "mocha test-dist/**/*.js --reporter ../../../common/tools/mocha-multi-reporter.js", | |||
There was a problem hiding this comment.
- The
--reporteroptions are exactly same for all the SDKs. - For local runs - we only need spec reporter and not junit-reporter.
spec reporteris the default reporter when we don't specify a reporter.- Both junit-reporter and spec reporter are required in the CI.
One proposal to refactor is to move the reporter options to the rush-yaml scripts where we trigger the test scripts instead of package.jsons if possible.
@KarishmaGhiya is checking if rush and our configs allow this. Logged followup #7934
HarshaNalluru
added
the
Client
| "use strict"; | ||
|
|
||
| const Mocha = require("mocha"); | ||
| const MochaJUnitReporter = require("mocha-junit-reporter"); |
There was a problem hiding this comment.
One thing to note here is that the mocha-multi-reporter.js file is not part of any project/utilities, it is an independent file and doesn't have a package.json of its own.
Placed here(under common/tools/ folder) only so that all the SDKs can leverage.
SDKs are expected to import mocha and mocha-junit-reporter as devDependencies and then use this .js file as the reporter in the mocha test command in package.json.
common/config/rush/pnpm-lock.yaml
Outdated
| @@ -49,7 +49,7 @@ packages: | |||
| is-buffer: 2.0.4 | |||
| jssha: 2.3.1 | |||
| process: 0.11.10 | |||
| rhea: 1.0.19 | |||
| rhea: 1.0.20 | |||
There was a problem hiding this comment.
How did removing dependency on mocha-multi result in rhea version getting updated?
Is this a result of running more than just rush update?
There was a problem hiding this comment.
No, this is due to rush update --full.
There was a problem hiding this comment.
If a library is removed, pnpm-lock file is unaffected with rush update.
There was a problem hiding this comment.
I thought the guidance was not to run rush update --full as part of regular PRs and that this would be done in frequent intervals by the Eng Sys team. This keeps the changes in the rest of the PR and the changes to lock file in sync
There was a problem hiding this comment.
Is the guidance that we should have a separate PR that only does update --full then? Otherwise we won't be able to shake removed deps out.
There was a problem hiding this comment.
If we need to remove a dep ASAP, then yes I would recommend a second PR that only does "update --full". Anyone on the JS team can create such a PR, or you can ask @KarishmaGhiya to do it on your behalf.
Alternatively, @KarishmaGhiya should be running "update --full" about once per week, and in most cases it should be fine to wait until this weekly update to remove unused deps.
There was a problem hiding this comment.
Of course, if a PR needs "update --full" to be green, then the PR should include an "update --full" commit in the same PR.
There was a problem hiding this comment.
The reason is that update --full updates all packages to semver latest, which often breaks something (and will often be unrelated to any other changes in your PR). So it's safest to isolate update --full into a separate PR and ensure we run core tests on it.
…com/HarshaNalluru/azure-sdk-for-js into harshan/custom-mocha-multi-reporter
HarshaNalluru
requested review from
richardpark-msft
and removed request for
richardpark-msft and
XiaoningLiu
Issue
mochatest runner, we leveragemocha-multilibrary to make use of multiple mocha reportersmocha-multilibrary doesn't supportmocha@7.0.0- latest(released a couple months ago).mocha@7.0.0would mean that we can't take dependence onmocha-multilibrarySolution
mocha-multiis being used to plug in multiple reporters(specandjunit-reporters specifically) during the test runsmocha@7.0.0upgrade, build a custom mocha reporter to invokespecandjunit-reporters and not rely onmocha-multiChanges in this PR
common/tools/that invokesspecandmocha-junit-reporter(and consequently getting rid of the dependence onmocha-multi)mocha-multi--reporter mocha-multi --reporter-options spec=-,mocha-junit-reporter=---reporter ../../../common/tools/mocha-multi-reporter.jsmocha-multifrom the devDependencies for all the SDKs in the repo"ts-mocha": "^6.0.0"for event-hubs which was also causing issues withmocha@7.0unit-testcommands of the@azure/eslint-plugin-azure-sdkto fix build failures caused by the missingunit-test:node