Skip to content

Replace console logs with logger #511

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

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

BelfordZ
Copy link
Contributor

@BelfordZ BelfordZ commented May 22, 2025

User description

Summary

  • use Context.logger.getLogger('main').error in getStreamWithTypeCheck and requestErrorHandler
  • adjust unit tests to check logger usage instead of console

Testing

  • npm test

PR Type

Bug fix, Tests


Description

  • Replace console.log with main logger error in helpers.

  • Update unit tests to mock and verify logger usage.

  • Add mocking for Context.logger in test setup.

  • Ensure error messages use logger, not console.


Changes walkthrough 📝

Relevant files
Bug fix
Helpers.ts
Use main logger for error reporting in helpers                     

src/types/Helpers.ts

  • Replaced console.log with Context.logger.getLogger('main').error for
    error reporting.
  • Updated error handling to use logger if available.
  • Removed direct console output for error cases.
  • +15/-2   
    Tests
    Helpers.test.ts
    Update tests to mock and verify logger usage                         

    test/unit/src/types/Helpers.test.ts

  • Mocked Context.logger and its getLogger method for tests.
  • Updated assertions to check logger usage instead of console.
  • Removed/modified tests that relied on console.log.
  • Ensured error handling is validated via logger mocks.
  • +16/-8   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logger Fallback

    The new logger usage in getStreamWithTypeCheck and requestErrorHandler only logs if Context.logger?.getLogger('main') is available. If the logger is undefined or misconfigured, errors may be silently ignored. Consider whether a fallback or warning is needed in such cases.

    /* prettier-ignore */
    const logger = Context.logger?.getLogger('main')
    if (logger) {
      logger.error(
        `Invalid request stream: expected: ${typeId} actual: ${requestType}. ${customErrorLog ? customErrorLog : ''}`
      )
    }
    Test Mock Cleanup

    The test suite mocks Context.logger and its methods globally. Ensure that mocks are properly reset or restored between tests to avoid cross-test contamination, especially if other tests rely on the same context.

    jest.mock('../../../../src/p2p/Context', () => {
      const mockLoggerError = jest.fn()
      const mockGetLogger = jest.fn(() => ({ error: mockLoggerError }))
      return {
        logger: { getLogger: mockGetLogger },
        __mockLoggerError: mockLoggerError,
        __mockGetLogger: mockGetLogger,
      }
    })
    const { __mockLoggerError: mockLoggerError, __mockGetLogger: mockGetLogger } = require('../../../../src/p2p/Context')

    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

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

    Successfully merging this pull request may close these issues.

    1 participant