fix: handle skipped audits and groups #911
Open
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
hanna-skryl
changed the title
hanna-skryl
force-pushed
the
fix/invalid-category-references
branch
from
8610bf6 to
884f53c
Compare
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared target commit 29a4de4 with source commit 4777a83. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories
👍 1 group improved, 👎 1 group regressed, 👍 4 audits improved, 👎 4 audits regressed, 12 audits changed without impacting score🗃️ Groups
15 other groups are unchanged. 🛡️ Audits
568 other audits are unchanged. |
hanna-skryl
force-pushed
the
fix/invalid-category-references
branch
from
1e29603 to
10e761b
Compare
matejchalk
reviewed
Jan 8, 2025
Comment on lines
+20
to
33
| it('should mark audits in onlyAudits as not skipped', () => { | ||
| const pluginConfig = lighthousePlugin('https://code-pushup-portal.com', { | ||
| onlyAudits: ['first-contentful-paint'], | ||
| }); | ||
|
|
||
| expect(() => pluginConfigSchema.parse(pluginConfig)).not.toThrow(); | ||
|
|
||
| expect(pluginConfig.audits[0]).toEqual( | ||
| expect( | ||
| pluginConfig.audits.find(({ slug }) => slug === 'first-contentful-paint'), | ||
| ).toEqual( | ||
| expect.objectContaining({ | ||
| slug: 'first-contentful-paint', | ||
| isSkipped: false, | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I'm missing some assertion that the other audits have isSkipped: true. (Same for groups.)
Comment on lines
+162
to
+172
| const weightMap = new Map<string, number>(); | ||
| plugin.groups.forEach(group => { | ||
| group.refs.forEach(ref => { | ||
| weightMap.set(ref.slug, (weightMap.get(ref.slug) ?? 0) + ref.weight); | ||
| }); | ||
| }); | ||
| const totalWeight = plugin.audits.reduce( | ||
| (sum, audit) => sum + (weightMap.get(audit.slug) ?? 0), | ||
| 0, | ||
| ); | ||
| return totalWeight === 0; |
There was a problem hiding this comment.
This implementation doesn't take into account that an audit can be referenced by multiple groups within the plugin. I would simply sum up the weights for each group, and then check if some group has a total weight of 0.
Comment on lines
+87
to
+102
| const invalidArgs = [ | ||
| { option: 'onlyCategories', args: onlyCategories ?? [] }, | ||
| { option: 'skipCategories', args: skipCategories ?? [] }, | ||
| ].filter(({ args }) => | ||
| args.some(arg => skippedCategories.some(({ slug }) => slug === arg)), | ||
| ); | ||
| if (invalidArgs.length > 0) { | ||
| throw new OptionValidationError( | ||
| invalidArgs | ||
| .map( | ||
| ({ option, args }) => | ||
| `The --${option} argument references skipped categories: ${args.join(', ')}`, | ||
| ) | ||
| .join('. '), | ||
| ); | ||
| } |
There was a problem hiding this comment.
I wouldn't throw validation errors in this case. Skipping an already skipped category seems harmless to me. And we could simply remove skipped categories from onlyCategories. Only if all of onlyCategories are skipped would we consider that a validation error, because that leaves us with nothing to run. But other than that, I would just log a warning. The "skipping already skipped" warning should probably be behind verbose flag as well. 🤔
| .map(({ isSkipped, ...props }) => props); | ||
| } | ||
|
|
||
| export function processPlugins(plugins: PluginConfig[]): PluginConfig[] { |
There was a problem hiding this comment.
The name processPlugins isn't very self-explanatory:
Suggested change
| export function processPlugins(plugins: PluginConfig[]): PluginConfig[] { | |
| export function filterSkippedInPlugins(plugins: PluginConfig[]): PluginConfig[] { |
matejchalk
reviewed
Jan 8, 2025
Comment on lines
+104
to
+106
| isSkipped: isSkippedDescription | ||
| ? isSkippedSchema.describe(isSkippedDescription) | ||
| : isSkippedSchema, |
There was a problem hiding this comment.
Could you please also re-generate the Markdown docs (npx nx generate-docs models)?
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.
Fixes #903
This fix enhances the filtering of report results by audits and groups through the core configuration (currently implemented for the Lighthouse plugin only).
The
isSkippedproperty is introduced in thePluginConfigmetadata for audits and groups, ensuring the complete set of plugin audits and groups is retained duringCoreConfigvalidation to prevent unexpected errors. Filtering is now deferred to the filter middleware, which accurately handles skipped items by removing categories that contain only skipped or zero-weight references.