MLE-24763 Enabling logging test and removing winston#997
Conversation
rjrudin
requested review from
BillFarber,
anu3990 and
stevebio
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR removes the Winston logging library dependency from the codebase, retaining only Bunyan for logging functionality. The changes simplify the test suite and eliminate unnecessary package dependencies.
- Removed Winston logging tests and dependencies from the test suite
- Cleaned up package.json by removing winston and related color dependencies
- Updated Jenkinsfile to run previously excluded logging tests
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test-basic/logging.js | Removed Winston-specific logging tests and imports, keeping only Bunyan tests |
| package.json | Removed winston dependency and related color package overrides |
| Jenkinsfile | Removed test exclusions for logging tests, allowing them to run in CI |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }); | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] There are excessive blank lines at the end of the file. Consider removing the extra blank line on line 50 to maintain consistent formatting.
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
rjrudin
changed the title
| "color-name": "2.0.0", | ||
| "color-string": "2.1.0", | ||
| "cross-spawn": "7.0.6", | ||
| "debug": "4.3.6", |
There was a problem hiding this comment.
These were part of fix for npm supply chain attack.
anu3990
left a comment
anu3990
left a comment
There was a problem hiding this comment.
These were part of fix for npm supply chain attack. Also, what is the motivation for this PR ?
rjrudin
force-pushed
the
feature/24763-logging-test
branch
from
2cf0cd1 to
c9035a5
Compare
|
The mocha regex also excluded tests with name 'archivePath' and that matches the test file documents-temporal-protect.js. Not sure why that was excluded, maybe not documented as to why. But seems unrelated to removal of winston logging? |
There are no files containing |
This includes the logging.js test in the suite of tests, but without Winston, as testing just with Bunyan is sufficient for testing the feature. By removing Winston, we're able to remove two of the libraries that required overrides. Nothing else depends on those two libraries, so the overrides can be safely removed. |
rjrudin
force-pushed
the
feature/24763-logging-test
branch
from
c9035a5 to
b9dd9d5
Compare
Only using bunyan in logging.js, and that test seems very safe to run, so it's no longer excluded by Jenkinsfile. Removed the two "test-complete" logging tests that weren't actually doing anything. Removing winston allows for the "color" and "color-string" overrides to be removed as well as nothing else depends on them.
rjrudin
force-pushed
the
feature/24763-logging-test
branch
from
b9dd9d5 to
9314dab
Compare
Only using bunyan in logging.js, and that test seems very safe to run, so it's no longer excluded by Jenkinsfile.
Removing winston allows for the "color" and "color-string" overrides to be removed as well as nothing else depends on them.