Open
Conversation
|
View your CI Pipeline Execution ↗ for commit 058c9c1
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit ad83c42
☁️ Nx Cloud last updated this comment at |
@code-pushup/ci
@code-pushup/core
@code-pushup/create-cli
@code-pushup/models
@code-pushup/nx-plugin
@code-pushup/cli
@code-pushup/axe-plugin
@code-pushup/coverage-plugin
@code-pushup/eslint-plugin
@code-pushup/js-packages-plugin
@code-pushup/jsdocs-plugin
@code-pushup/lighthouse-plugin
@code-pushup/typescript-plugin
@code-pushup/utils
commit: |
Contributor
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit 341273e with previous commit 7b6e72c. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👍 1 group improved, 👎 2 groups regressed, 👍 5 audits improved, 👎 5 audits regressed, 18 audits changed without impacting score🗃️ Groups
31 other groups are unchanged. 🛡️ Audits
651 other audits are unchanged. |
Contributor
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared current commit 341273e with previous commit 7b6e72c. 💼 Project
|
| 🏷️ Category | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|
| Documentation | 🟡 61 | 🟡 60 |  |
| Code coverage | 🟢 95 | 🟢 95 | |
4 other categories are unchanged.
👎 2 groups regressed, 👍 2 audits improved, 👎 4 audits regressed
🗃️ Groups
| 🔌 Plugin | 🗃️ Group | ⭐ Previous score | ⭐ Current score | 🔄 Score change |
|---|---|---|---|---|
| JSDocs coverage | Documentation coverage | 🟡 61 | 🟡 60 | |
| Code coverage | Code coverage metrics | 🟢 95 | 🟢 95 | |
13 other groups are unchanged.
🛡️ Audits
| 🔌 Plugin | 🛡️ Audit | 📏 Previous value | 📏 Current value | 🔄 Value change |
|---|---|---|---|---|
| JSDocs coverage | Variables coverage | 🟥 50 undocumented variables | 🟥 49 undocumented variables |  |
| JSDocs coverage | Types coverage | 🟨 55 undocumented types | 🟨 55 undocumented types |  |
| JSDocs coverage | Functions coverage | 🟥 243 undocumented functions | 🟥 243 undocumented functions | |
| JSDocs coverage | Properties coverage | 🟥 39 undocumented properties | 🟥 40 undocumented properties |  |
| Code coverage | Branch coverage | 🟩 91.9 % | 🟩 91.8 % |  |
| Code coverage | Line coverage | 🟩 97.8 % | 🟩 97.8 % |  |
438 other audits are unchanged.
13 other projects are unchanged.
This was referenced Feb 3, 2026
hanna-skryl
reviewed
Feb 3, 2026
| const result = serializeTraceEvent(event); | ||
|
|
||
| expect(result).toStrictEqual({ | ||
| expect(typeof result).toBe('string'); |
Collaborator
There was a problem hiding this comment.
I'd recommend using either Vitest's .toBeTypeOf('string') or jest-extended's .toBeString() instead. This also applies in other places in this PR.
Suggested change
| expect(typeof result).toBe('string'); | |
| expect(result).toBeString(); |
Collaborator
There was a problem hiding this comment.
Also, is this check really necessary (here and everywhere else)? If the toStrictEqual below passes, the produced result is correct, isn't it?
Collaborator
There was a problem hiding this comment.
There are still seven instances of a check similar to expect(typeof result).toBe('string');.
Co-authored-by: Hanna Skryl <[email protected]>
Co-authored-by: Hanna Skryl <[email protected]>
Co-authored-by: Hanna Skryl <[email protected]>
Co-authored-by: Hanna Skryl <[email protected]>
hanna-skryl
reviewed
Feb 4, 2026
| metadata?: Partial<TraceMetadata>; | ||
| }): TraceEventContainer => ({ | ||
| traceEvents: opt.traceEvents, | ||
| traceEvents: opt.traceEvents.map(encodeEvent), |
Collaborator
There was a problem hiding this comment.
createTraceFile now encodes events internally, but generateTraceContent, where it's used, immediately discards those encoded events by overwriting traceEvents 🤔
Comment on lines
529
to
547
| it('should handle zero values', () => { | ||
| const result = getShardId(); | ||
| const counter = { next: () => 1 }; | ||
| const result = getUniqueInstanceId(counter); | ||
| expect(result).toStartWith('20231114-221320-000.'); | ||
| }); | ||
|
|
||
| it('should handle negative timestamps', () => { | ||
| const result = getShardId(); | ||
| const counter = { next: () => 1 }; | ||
| const result = getUniqueInstanceId(counter); | ||
|
|
||
| expect(result).toStartWith('20231114-221320-000.'); | ||
| }); | ||
|
|
||
| it('should handle large timestamps', () => { | ||
| const result = getShardId(); | ||
| const counter = { next: () => 1 }; | ||
| const result = getUniqueInstanceId(counter); | ||
|
|
||
| expect(result).toStartWith('20231114-221320-000.'); | ||
| }); |
Collaborator
There was a problem hiding this comment.
The content of these tests is identical, but their descriptions differ. Is there something implied I'm missing?
| ); | ||
| }); | ||
|
|
||
| describe('ID_PATTERNS', () => { |
Collaborator
There was a problem hiding this comment.
There are three it.each() test cases in the ID_PATTERNS suite with only one input. Is this necessary?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related:
This PR includes:
Followup: