Skip to content

Conversation

maverick1872
Copy link
Member

@maverick1872 maverick1872 commented Aug 3, 2025

Migrate Winston Test Suite from Mocha to Jest

This PR fully migrates Winston's test suite from Mocha to Jest to provide a more fully featured testing framework

Warning

The File Transport tests while passing in their current form, illustrate in my opinion
some potential issues in how it currently functions.

  • You can see the FIX: comments on a couple of assertions that I presume are correct but are currently failing.
  • The intersection of behavior with with the maxsize and lazy options result in behavior I found unexpected. e.g. If the rotation is not lazy, it will drop a log file resulting in 1 less populated log file than I would expect
  • The maxsize option appears not to be strictly enforced on a per-file basis. This is probably desirable as in maxsize is likely on a best effort basis. I make this presumption given we wouldn't want to split a log payload across two files, nor do I think we should truncate it. The documentation for the Transport does not state either way though so look to someone with more familiarity than I about original intent.d

Summary

  • Refactored failing tests to be Jest-compatible
    • Primarily this was just modify before(), after(), this.timeout() to beforeAll(), afterAll(), and jest.setTimeout()
  • Moved the winston.test.js from integration to unit. This felt more appropriate given the contents of the test.
  • Refactored some tests for clarity or to resolve testing framework incompatabilities
    • Jest intercepts all stderr and pipes to stdout, so some tests have been updated to rely on Spies of console.error
    • Introduce async/await in some tests to reduce function nesting.
    • Combine File Archive tests and remove tests that I felt were redundant. This ensures these tests run sequentially as they're in the same file. This helps to avoid the potential of race conditions. Further improvements could likely be made here. Lastly these are not what I would label as unit tests and would likely advocate moving them to the integration suite given they're actually writing files to the FS.

Test Plan

  • All tests are passing in CI environment
  • Manual verification of test coverage shows equivalent or better coverage compared to Mocha
  • Verified file system tests properly clean up temporary files
  • Confirmed logging tests produce expected output formats

@maverick1872 maverick1872 marked this pull request as draft August 3, 2025 06:10
@indexzero
Copy link
Member

Appreciate the effort but this is not the direction the project is interested in going. Similar PRs that consider tap or the built-in node.js test runner would be considered. Thanks!

@indexzero indexzero closed this Aug 3, 2025
@indexzero
Copy link
Member

Philosophically I am a minimalist w.r.t. software design patterns & API surfaces (see: Robustness principal, also known as "Postel's law"). This philosophy is deeply embedded into the history & design of Node.js itself.

That said, those values must be balanced carefully with those that value volunteers like yourself @maverick1872 who step up to help in Keeping Important Code Alive, [SeattleJS 2015]

Before tools like claude, the former would have outweighed the outweighed the latter in this case as the decision to pursue large scale migrations such as mocha to jest were "one way doors".

I realized this morning that claude (and similar code generation tools such as gemini, codex, etc) turn these decisions into "two way doors" because this entire effort is only five days old.

This would have taken weeks if not months in the past.

All of this taken together has leads me to retract my previous objections @maverick1872 as long as we have a shared understanding as maintainers that we may revisit this decision in the future as it is now relatively trivial to accomplish.

@indexzero indexzero reopened this Aug 4, 2025
@maverick1872 maverick1872 force-pushed the migrate-to-jest branch 3 times, most recently from 6a4aac7 to 10dcb6d Compare August 9, 2025 20:26
@maverick1872 maverick1872 marked this pull request as ready for review August 9, 2025 21:12
@maverick1872
Copy link
Member Author

@indexzero @DABH do either of you know if coveralls can inform you as to where the coverage decrease occurred? Looks like it's most likely a byproduct of the consolidation of the File Transport tests. Happy to just undo the refactor but figured consolidation would ease some maintenance and clarity of what the tests are doing.

@DABH
Copy link
Contributor

DABH commented Aug 9, 2025

@maverick1872 Yeah just click the three dots next to the coverage/coveralls line in the little PR check area on this page and then click View Details. It should take you to the Coveralls site which lists individual files and their coverage changes you can look at. Thank you so much for the attention to detail here. Feel free to @ me when the PR is ready for review :)

Previously the exception handler tests were written in a way that
a child node process was spawned by invoking a script that would
throw an unhandled exception. This exception would invoke the exception
handler and result in an invocation of `process.exit()`. Because
of this these lines weren't being included in the coverage report
resulting in what appeared to be a coverage regression.

I opted to refactor the implementation to hopefully clarify what is being tested
and how. The helpers while useful for the implementation, added a level
of obfuscation that did not make it immediately obvious what the test
did exactly.
@maverick1872
Copy link
Member Author

@DABH @indexzero feel free to review when/if desired. I know it's large so I would advocate looking to the summary in the PR description which should hopefully make it clear what all has been done. Happy to have a more involved discussion/provide reasoning on any of my choices if desired.

@maverick1872 maverick1872 requested a review from Copilot August 10, 2025 19:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates Winston's test suite from Mocha to Jest, modernizing the testing infrastructure to provide enhanced testing capabilities and improved developer experience.

  • Converted all test files from Mocha syntax (before, after, this.timeout()) to Jest equivalents (beforeAll, afterAll, jest.setTimeout())
  • Consolidated redundant file transport tests into a single comprehensive test file with improved organization
  • Updated CI configuration and documentation to reflect the new Jest-based testing approach

Reviewed Changes

Copilot reviewed 36 out of 38 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/unit/winston/winston.test.js New comprehensive test file moved from integration, testing Winston's public API
test/unit/winston/transports/file.test.js Consolidated and modernized file transport tests with comprehensive coverage
test/unit/winston/transports/http.test.js Updated timeout syntax for Jest compatibility
test/unit/winston/logger.test.js Enhanced with new test cases and Jest-compatible error handling
test/unit/winston/log-exception.test.js Modernized exception handling tests with async/await patterns
package.json Updated dependencies and scripts to use Jest instead of Mocha
.github/workflows/ci.yml Updated CI pipeline for Jest-based testing workflow
Comments suppressed due to low confidence (4)

test/unit/winston/winston.test.js:24

  • [nitpick] The parameter name 'opts' is ambiguous. Consider renaming to 'options' for better clarity.
  });

Comment on lines +1240 to +1241
npm run test:test # Runs all integration tests
npm run test:test # Runs all integration tests
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command 'npm run test:test' appears to be incorrect. Based on the package.json scripts, it should be 'npm run test:integration'.

Suggested change
npm run test:test # Runs all integration tests
npm run test:test # Runs all integration tests
npm run test:integration # Runs all integration tests
npm run test:integration # Runs all integration tests

Copilot uses AI. Check for mistakes.

npm test # Runs all tests
npm run test:unit # Runs all Unit tests with coverage
npm run test:test # Runs all integration tests
npm run test:test # Runs all integration tests
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is duplicated and contains the same incorrect command as the previous line.

Suggested change
npm run test:test # Runs all integration tests

Copilot uses AI. Check for mistakes.

npm run test:unit # Runs all Unit tests with coverage
npm run test:test # Runs all integration tests
npm run test:test # Runs all integration tests
npm run typescript:test # Runs tests verifying Typescript types
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command 'npm run typescript:test' is incorrect. Based on the package.json scripts, it should be 'npm run test:typescript'.

Suggested change
npm run typescript:test # Runs tests verifying Typescript types
npm run test:typescript # Runs tests verifying Typescript types

Copilot uses AI. Check for mistakes.

```

All of the winston tests are written with [`jest`][jest]. Assertions use a mix of [`assume`][assume] and the built-in jest assertion library.
Copy link
Preview

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation references a '[jest]' link that is not defined at the bottom of the document, unlike the other reference links.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this once you've appeased Copilot's comments. Thanks again for your efforts here!

[Chris Alderson]: https://github.com/chrisalderson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to add yourself ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants