test(test-setup): add custom matchers for ui logger #923
+422
−125
Conversation
This file contains 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
github-actions
bot
added
🔬 testing
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared target commit c440d62 with source commit 1686dda. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👎 2 groups regressed, 👍 4 audits improved, 👎 6 audits regressed, 10 audits changed without impacting score🗃️ Groups
16 other groups are unchanged. 🛡️ Audits
576 other audits are unchanged. |
matejchalk
reviewed
Jan 17, 2025
Comment on lines
77
to
79
| expect(ui()).toHaveLoggedLevel('info'); | ||
| expect(ui()).toHaveLoggedMessage( | ||
| `Reports diff written to ${bold( |
There was a problem hiding this comment.
I would merge the level and message assertions into one. Apart from more convenient usage (IMHO), it will also ensure the same level-message pair is being checked.
Suggested change
| expect(ui()).toHaveLoggedLevel('info'); | |
| expect(ui()).toHaveLoggedMessage( | |
| `Reports diff written to ${bold( | |
| expect(ui()).toHaveLogged( | |
| 'info', | |
| `Reports diff written to ${bold( |
Comment on lines
317
to
326
| expect(ui()).toHaveLoggedNthLevel(1, 'info'); | ||
| expect(ui()).toHaveLoggedNthLevel(2, 'info'); | ||
| expect(ui()).toHaveLoggedNthMessage( | ||
| 1, | ||
| 'The --skipPlugins argument removed the following categories: c1, c2.', | ||
| ); | ||
| expect(ui()).toHaveLoggedNthMessage( | ||
| 2, | ||
| 'The --onlyPlugins argument removed the following categories: c1, c2.', | ||
| ); |
There was a problem hiding this comment.
Following up on my previous comment, I would merge the Nth assertions also:
Suggested change
| expect(ui()).toHaveLoggedNthLevel(1, 'info'); | |
| expect(ui()).toHaveLoggedNthLevel(2, 'info'); | |
| expect(ui()).toHaveLoggedNthMessage( | |
| 1, | |
| 'The --skipPlugins argument removed the following categories: c1, c2.', | |
| ); | |
| expect(ui()).toHaveLoggedNthMessage( | |
| 2, | |
| 'The --onlyPlugins argument removed the following categories: c1, c2.', | |
| ); | |
| expect(ui()).toHaveLoggedNth( | |
| 1, | |
| 'info', | |
| 'The --skipPlugins argument removed the following categories: c1, c2.', | |
| ); | |
| expect(ui()).toHaveLoggedNth( | |
| 2, | |
| 'info', | |
| 'The --onlyPlugins argument removed the following categories: c1, c2.', | |
| ); |
Comment on lines
29
to
33
| expect(ui()).not.toHaveLoggedMessageContaining('"$0":'); | ||
| expect(ui()).not.toHaveLoggedMessageContaining('"_":'); | ||
|
|
||
| expect(log).toContain('"outputDir": "destinationDir"'); | ||
| expect(log).not.toContain('"output-dir":'); | ||
| expect(ui()).toHaveLoggedMessageContaining('"outputDir": "destinationDir"'); | ||
| expect(ui()).not.toHaveLoggedMessageContaining('"output-dir":'); |
There was a problem hiding this comment.
Can we use expect.stringContaining('...') here instead? 🤔 Would be cool if we didn't need a specialized matcher for that.
Supporting the built-in string matchers would be most flexible, e.g. to use regexes instead of substrings we'd just use expect.stringMatching(/.../).
There was a problem hiding this comment.
That's a great idea! It turned out to be easier to implement than I expected 😁
Comment on lines
79
to
82
| const levels = actual.logger | ||
| .getRenderer() | ||
| .getLogs() | ||
| .map(({ message }) => extractLevel(message)); |
There was a problem hiding this comment.
The actual.logger.getRenderer().getLogs().map(({ message }) => message) logic could be extracted to a helper function.
Comment on lines
21
to
34
| export function extractLevel(log: string): LogLevel | null { | ||
| const match = removeColorCodes(log).match(/^\[\s*\w+\((?<level>\w+)\)\s*]/); | ||
| const level = match?.groups?.['level'] as LogLevel | undefined; | ||
| return level && LOG_LEVELS.has(level) ? level : null; | ||
| } | ||
|
|
||
| export function extractMessage(log: string): ExtractedMessage { | ||
| const match = log.match( | ||
| /^\[\s*\w+\((?<level>\w+)\)\s*]\s*(?<message>.+?(\.\s*)?)$/, | ||
| ); | ||
| const styledMessage = match?.groups?.['message'] ?? log; | ||
| const unstyledMessage = removeColorCodes(styledMessage); | ||
| return { styledMessage, unstyledMessage }; | ||
| } |
There was a problem hiding this comment.
This looks very robust 👍 Also well tested 🙂
hanna-skryl
force-pushed
the
feat/custom-ui-logger-matchers
branch
from
60d7b79 to
7363d50
Compare
BioPhoton
approved these changes
Jan 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #552
This PR adds custom matchers for
ui()logger.