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

Logger Tests Regression / Partial Reversion #758

Closed
1 task done
susumutomita opened this issue Jan 3, 2025 · 0 comments · Fixed by #759
Closed
1 task done

Logger Tests Regression / Partial Reversion #758

susumutomita opened this issue Jan 3, 2025 · 0 comments · Fixed by #759

Comments

@susumutomita
Copy link
Contributor

susumutomita commented Jan 3, 2025

Is there an existing issue for this?

  • I have searched the existing issues

SDK version

Using Lit JS SDK at commit 412264b and beyond (master / branches after PR #579 ).

Lit Network

Datil / or whichever network you’re using (if applicable).

Description of the bug/issue

After the refactoring in PR #579 , the LogLevel enum was supposed to be replaced by LOG_LEVEL constants from @lit-protocol/constants. However, subsequent merges caused a partial rollback (ancestor revert):

  • Commit: Merge branch 'master' into staging/v7 on Oct 3, 2024, reintroduced LogLevel references while also removing export { LOG_LEVEL }; in logger.ts.
  • This mismatch led to failing tests (TypeScript errors and incorrect timestamp comparisons), as well as confusion around whether LogLevel or LOG_LEVEL should be used.

Currently logger test fails with the error

npx nx run logger:test --coverage
:
 FAIL   logger  packages/logger/src/lib/logger.spec.ts
  ● Test suite failed to run

    packages/logger/src/lib/logger.spec.ts:1:10 - error TS2724: '"./logger"' has no exported member named 'LOG_LEVEL'. Did you mean 'LogLevel'?

    1 import { LOG_LEVEL, LogManager } from './logger';

We want to keep the code aligned with the overall project direction—to unify everything under LOG_LEVEL—but also maintain backwards compatibility in the short term (e.g., ensuring both LogLevel and LOG_LEVEL references don’t break the build).

Severity of the bug

Medium: The logger tests fail out-of-the-box, causing confusion and blocking straightforward usage or contributions in the logger package.

Steps To Reproduce

  1. In this environment (Node.js v20, Nx 17.x, etc.), check out the master or staging/v7 branch at or after commit c8801c5fe87cacee7....
  2. Run npx nx run logger:test --coverage or yarn tools --test --unit (depending on your setup).
  3. See TypeScript errors:
npx nx run logger:test --coverage
:
 FAIL   logger  packages/logger/src/lib/logger.spec.ts
  ● Test suite failed to run

    packages/logger/src/lib/logger.spec.ts:1:10 - error TS2724: '"./logger"' has no exported member named 'LOG_LEVEL'. Did you mean 'LogLevel'?

    1 import { LOG_LEVEL, LogManager } from './logger';

Or find that tests referencing LogLevel fail if the code exports only LOG_LEVEL, etc.

Link to code

Anything else?

Proposed fix

  • Reintroduce export { LOG_LEVEL } in logger.ts, ensuring backward compatibility for any remaining references to LogLevel.
  • Update the tests to consistently use LOG_LEVEL.DEBUG (etc.) instead of LogLevel.DEBUG, or vice-versa if the old enum is still needed.
  • Ultimately unify everything under LOG_LEVEL per the original refactor’s intent.

Context

  • I tested a local branch that modifies logger.spec.ts to rely fully on LOG_LEVEL, while still exporting both LOG_LEVEL and LogLevel for backward compatibility.
  • After these changes, npx nx run logger:test --coverage passes with no TS errors and the test that checks creation timestamps is fixed to compare numeric vs. string properly.
@susumutomita susumutomita mentioned this issue Jan 3, 2025
12 tasks
@susumutomita susumutomita changed the title [Team Name] Logger Tests Regression / Partial Reversion Logger Tests Regression / Partial Reversion Jan 3, 2025
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 a pull request may close this issue.

1 participant