From b04e86a111f4db09216d0fa10aa4517b088c7313 Mon Sep 17 00:00:00 2001 From: hanna-skryl Date: Thu, 7 Nov 2024 12:45:43 -0500 Subject: [PATCH 1/4] refactor: standardize undefined handling for categories --- .../cli/src/lib/autorun/autorun-command.ts | 2 +- .../cli/src/lib/collect/collect-command.ts | 2 +- .../core-config.integration.test.ts | 4 +-- .../implementation/core-config.middleware.ts | 2 -- .../lib/implementation/filter.middleware.ts | 33 +++++++++++-------- .../filter.middleware.unit.test.ts | 2 -- .../src/lib/implementation/filter.model.ts | 11 ++----- .../validate-filter-options.utils.ts | 6 ++-- ...validate-filter-options.utils.unit.test.ts | 21 +++++++++--- packages/core/src/lib/collect-and-persist.ts | 5 +-- .../src/lib/collect-and-persist.unit.test.ts | 3 -- packages/core/src/lib/history.ts | 4 +-- packages/core/src/lib/history.unit.test.ts | 1 - .../collect.integration.test.ts | 1 - .../core/src/lib/implementation/collect.ts | 4 +-- .../lib/implementation/compare-scorables.ts | 4 +-- .../src/lib/implementation/report-to-gql.ts | 2 +- packages/models/src/lib/core-config.ts | 11 +++---- .../models/src/lib/implementation/utils.ts | 8 ++--- packages/models/src/lib/report.ts | 12 +++---- packages/models/src/lib/report.unit.test.ts | 1 - .../utils/perf/score-report/optimized0.ts | 2 +- .../generate-md-report-categoy-section.ts | 4 +-- .../src/lib/reports/generate-md-report.ts | 26 ++++++++++----- .../reports/generate-md-report.unit.test.ts | 4 +-- .../log-stdout-summary.integration.test.ts | 4 +-- .../src/lib/reports/log-stdout-summary.ts | 22 ++++++++----- .../reports/log-stdout-summary.unit.test.ts | 6 ++-- packages/utils/src/lib/reports/scoring.ts | 2 +- .../src/lib/reports/scoring.unit.test.ts | 6 ++-- .../lib/reports/sorting.integration.test.ts | 6 ++-- packages/utils/src/lib/reports/sorting.ts | 2 +- packages/utils/src/lib/reports/types.ts | 2 +- .../test-utils/src/lib/utils/report.mock.ts | 1 - 34 files changed, 118 insertions(+), 108 deletions(-) diff --git a/packages/cli/src/lib/autorun/autorun-command.ts b/packages/cli/src/lib/autorun/autorun-command.ts index ecdc16d89..dd0dfeb53 100644 --- a/packages/cli/src/lib/autorun/autorun-command.ts +++ b/packages/cli/src/lib/autorun/autorun-command.ts @@ -41,7 +41,7 @@ export function yargsAutorunCommandObject() { await collectAndPersistReports(optionsWithFormat); collectSuccessfulLog(); - if (options.categories.length === 0) { + if (!options.categories || options.categories.length === 0) { renderConfigureCategoriesHint(); } diff --git a/packages/cli/src/lib/collect/collect-command.ts b/packages/cli/src/lib/collect/collect-command.ts index b58e88f1a..fc51781b1 100644 --- a/packages/cli/src/lib/collect/collect-command.ts +++ b/packages/cli/src/lib/collect/collect-command.ts @@ -24,7 +24,7 @@ export function yargsCollectCommandObject(): CommandModule { await collectAndPersistReports(options); collectSuccessfulLog(); - if (options.categories.length === 0) { + if (!options.categories || options.categories.length === 0) { renderConfigureCategoriesHint(); } diff --git a/packages/cli/src/lib/implementation/core-config.integration.test.ts b/packages/cli/src/lib/implementation/core-config.integration.test.ts index 75972c9e6..94ea12093 100644 --- a/packages/cli/src/lib/implementation/core-config.integration.test.ts +++ b/packages/cli/src/lib/implementation/core-config.integration.test.ts @@ -144,7 +144,7 @@ describe('parsing values from CLI and middleware', () => { }); }); - it('should set empty array for categories if not given in config file', async () => { + it('should keep categories undefined if not given in config file', async () => { const { categories } = await yargsCli( ['--config=./no-category.config.ts'], { @@ -153,7 +153,7 @@ describe('parsing values from CLI and middleware', () => { }, ).parseAsync(); - expect(categories).toEqual([]); + expect(categories).toBeUndefined(); }); it('should accept an upload configuration with CLI overrides', async () => { diff --git a/packages/cli/src/lib/implementation/core-config.middleware.ts b/packages/cli/src/lib/implementation/core-config.middleware.ts index c51dd1da8..7be073286 100644 --- a/packages/cli/src/lib/implementation/core-config.middleware.ts +++ b/packages/cli/src/lib/implementation/core-config.middleware.ts @@ -32,7 +32,6 @@ export async function coreConfigMiddleware< const { persist: rcPersist, upload: rcUpload, - categories: rcCategories, ...remainingRcConfig } = importedRc; const upload = @@ -56,7 +55,6 @@ export async function coreConfigMiddleware< ), }, ...(upload != null && { upload }), - categories: rcCategories ?? [], ...remainingRcConfig, ...remainingCliOptions, }; diff --git a/packages/cli/src/lib/implementation/filter.middleware.ts b/packages/cli/src/lib/implementation/filter.middleware.ts index 1e3ed00b7..716f6a2cc 100644 --- a/packages/cli/src/lib/implementation/filter.middleware.ts +++ b/packages/cli/src/lib/implementation/filter.middleware.ts @@ -1,4 +1,4 @@ -import type { CategoryConfig, PluginConfig } from '@code-pushup/models'; +import type { CoreConfig } from '@code-pushup/models'; import { filterItemRefsBy } from '@code-pushup/utils'; import type { FilterOptions, Filterables } from './filter.model'; import { @@ -12,7 +12,7 @@ export function filterMiddleware( ): T { const { plugins, - categories = [], + categories, skipCategories = [], onlyCategories = [], skipPlugins = [], @@ -26,7 +26,7 @@ export function filterMiddleware( skipPlugins.length === 0 && onlyPlugins.length === 0 ) { - return { ...originalProcessArgs, categories }; + return originalProcessArgs; } handleConflictingOptions('categories', onlyCategories, skipCategories); @@ -44,9 +44,11 @@ export function filterMiddleware( onlyPlugins, verbose, ); - const finalCategories = filterItemRefsBy(filteredCategories, ref => - filteredPlugins.some(plugin => plugin.slug === ref.plugin), - ); + const finalCategories = filteredCategories + ? filterItemRefsBy(filteredCategories, ref => + filteredPlugins.some(plugin => plugin.slug === ref.plugin), + ) + : filteredCategories; validateFinalState( { categories: finalCategories, plugins: filteredPlugins }, @@ -80,8 +82,12 @@ function applyCategoryFilters( skipCategories: string[], onlyCategories: string[], verbose: boolean, -): CategoryConfig[] { - if (skipCategories.length === 0 && onlyCategories.length === 0) { +): CoreConfig['categories'] { + if ( + (skipCategories.length === 0 && onlyCategories.length === 0) || + !categories || + categories.length === 0 + ) { return categories; } validateFilterOption( @@ -102,7 +108,7 @@ function applyPluginFilters( skipPlugins: string[], onlyPlugins: string[], verbose: boolean, -): PluginConfig[] { +): CoreConfig['plugins'] { const filteredPlugins = filterPluginsFromCategories({ categories, plugins, @@ -126,11 +132,12 @@ function applyPluginFilters( function filterPluginsFromCategories({ categories, plugins, -}: Filterables): PluginConfig[] { +}: Filterables): CoreConfig['plugins'] { + if (!categories || categories.length === 0) { + return plugins; + } const validPluginSlugs = new Set( categories.flatMap(category => category.refs.map(ref => ref.plugin)), ); - return categories.length > 0 - ? plugins.filter(plugin => validPluginSlugs.has(plugin.slug)) - : plugins; + return plugins.filter(plugin => validPluginSlugs.has(plugin.slug)); } diff --git a/packages/cli/src/lib/implementation/filter.middleware.unit.test.ts b/packages/cli/src/lib/implementation/filter.middleware.unit.test.ts index 9a5458d1f..c81bc9e86 100644 --- a/packages/cli/src/lib/implementation/filter.middleware.unit.test.ts +++ b/packages/cli/src/lib/implementation/filter.middleware.unit.test.ts @@ -22,7 +22,6 @@ describe('filterMiddleware', () => { }), ).toStrictEqual({ plugins: [{ slug: 'p1', audits: [{ slug: 'a1-p1' }] }], - categories: [], }); }); @@ -89,7 +88,6 @@ describe('filterMiddleware', () => { const { plugins } = filterMiddleware({ ...option, plugins: [{ slug: 'p1' }, { slug: 'p2' }] as PluginConfig[], - categories: [], }); expect(plugins).toStrictEqual([expect.objectContaining(expected)]); }, diff --git a/packages/cli/src/lib/implementation/filter.model.ts b/packages/cli/src/lib/implementation/filter.model.ts index 467f14e6d..5d36bcdf9 100644 --- a/packages/cli/src/lib/implementation/filter.model.ts +++ b/packages/cli/src/lib/implementation/filter.model.ts @@ -1,9 +1,5 @@ import type { GlobalOptions } from '@code-pushup/core'; -import type { - CategoryConfig, - CoreConfig, - PluginConfig, -} from '@code-pushup/models'; +import type { CoreConfig } from '@code-pushup/models'; export type FilterOptions = Partial & Pick & @@ -17,7 +13,4 @@ export type FilterOptionType = | 'skipPlugins' | 'onlyPlugins'; -export type Filterables = { - categories: CategoryConfig[]; - plugins: PluginConfig[]; -}; +export type Filterables = Pick; diff --git a/packages/cli/src/lib/implementation/validate-filter-options.utils.ts b/packages/cli/src/lib/implementation/validate-filter-options.utils.ts index 1b87cb79c..d4baa8948 100644 --- a/packages/cli/src/lib/implementation/validate-filter-options.utils.ts +++ b/packages/cli/src/lib/implementation/validate-filter-options.utils.ts @@ -11,7 +11,7 @@ export class OptionValidationError extends Error {} export function validateFilterOption( option: FilterOptionType, - { plugins, categories }: Filterables, + { plugins, categories = [] }: Filterables, { itemsToFilter, verbose }: { itemsToFilter: string[]; verbose: boolean }, ): void { const itemsToFilterSet = new Set(itemsToFilter); @@ -59,9 +59,9 @@ export function validateFinalState( filteredItems: Filterables, originalItems: Filterables, ): void { - const { categories: filteredCategories, plugins: filteredPlugins } = + const { categories: filteredCategories = [], plugins: filteredPlugins } = filteredItems; - const { categories: originalCategories, plugins: originalPlugins } = + const { categories: originalCategories = [], plugins: originalPlugins } = originalItems; if ( filteredCategories.length === 0 && diff --git a/packages/cli/src/lib/implementation/validate-filter-options.utils.unit.test.ts b/packages/cli/src/lib/implementation/validate-filter-options.utils.unit.test.ts index 2bf6f0ccf..f6f0be6c3 100644 --- a/packages/cli/src/lib/implementation/validate-filter-options.utils.unit.test.ts +++ b/packages/cli/src/lib/implementation/validate-filter-options.utils.unit.test.ts @@ -106,7 +106,6 @@ describe('validateFilterOption', () => { { slug: 'p1', audits: [{ slug: 'a1-p1' }] }, { slug: 'p2', audits: [{ slug: 'a1-p2' }] }, ] as PluginConfig[], - categories: [], }, { itemsToFilter: ['p1'], verbose: false }, ); @@ -145,7 +144,6 @@ describe('validateFilterOption', () => { { slug: 'p2', audits: [{ slug: 'a1-p2' }] }, { slug: 'p3', audits: [{ slug: 'a1-p3' }] }, ] as PluginConfig[], - categories: [], }, { itemsToFilter: ['p4', 'p5'], verbose: false }, ); @@ -165,12 +163,12 @@ describe('validateFilterOption', () => { expect(() => { validateFilterOption( 'skipPlugins', - { plugins: allPlugins, categories: [] }, + { plugins: allPlugins }, { itemsToFilter: ['plugin1'], verbose: false }, ); validateFilterOption( 'onlyPlugins', - { plugins: allPlugins, categories: [] }, + { plugins: allPlugins }, { itemsToFilter: ['plugin3'], verbose: false }, ); }).toThrow( @@ -320,6 +318,21 @@ describe('validateFinalState', () => { validateFinalState(filteredItems, originalItems); }).toThrow(expect.any(OptionValidationError)); }); + + it('should perform validation without throwing an error when categories are missing', () => { + const filteredItems = { + plugins: [{ slug: 'p1', audits: [{ slug: 'a1-p1' }] }] as PluginConfig[], + }; + const originalItems = { + plugins: [ + { slug: 'p1', audits: [{ slug: 'a1-p1' }] }, + { slug: 'p2', audits: [{ slug: 'a1-p2' }] }, + ] as PluginConfig[], + }; + expect(() => { + validateFinalState(filteredItems, originalItems); + }).not.toThrow(); + }); }); describe('getItemType', () => { diff --git a/packages/core/src/lib/collect-and-persist.ts b/packages/core/src/lib/collect-and-persist.ts index b1ff94648..036479a72 100644 --- a/packages/core/src/lib/collect-and-persist.ts +++ b/packages/core/src/lib/collect-and-persist.ts @@ -13,8 +13,9 @@ import { collect } from './implementation/collect'; import { logPersistedResults, persistReport } from './implementation/persist'; import type { GlobalOptions } from './types'; -export type CollectAndPersistReportsOptions = Required< - Pick +export type CollectAndPersistReportsOptions = Pick< + CoreConfig, + 'plugins' | 'categories' > & { persist: Required } & Partial; export async function collectAndPersistReports( diff --git a/packages/core/src/lib/collect-and-persist.unit.test.ts b/packages/core/src/lib/collect-and-persist.unit.test.ts index 1bfa017b1..e36529694 100644 --- a/packages/core/src/lib/collect-and-persist.unit.test.ts +++ b/packages/core/src/lib/collect-and-persist.unit.test.ts @@ -46,7 +46,6 @@ describe('collectAndPersistReports', () => { it('should call collect and persistReport with correct parameters in non-verbose mode', async () => { const sortedScoredReport = sortReport(scoreReport(MINIMAL_REPORT_MOCK)); const nonVerboseConfig: CollectAndPersistReportsOptions = { - categories: [], ...MINIMAL_CONFIG_MOCK, persist: { outputDir: 'output', @@ -69,7 +68,6 @@ describe('collectAndPersistReports', () => { date: expect.stringMatching(ISO_STRING_REGEXP), duration: 666, commit: expect.any(Object), - categories: expect.any(Array), plugins: expect.any(Array), }, sortedScoredReport, @@ -87,7 +85,6 @@ describe('collectAndPersistReports', () => { it('should call collect and persistReport with correct parameters in verbose mode', async () => { const sortedScoredReport = sortReport(scoreReport(MINIMAL_REPORT_MOCK)); const verboseConfig: CollectAndPersistReportsOptions = { - categories: [], ...MINIMAL_CONFIG_MOCK, persist: { outputDir: 'output', diff --git a/packages/core/src/lib/history.ts b/packages/core/src/lib/history.ts index 8c6e0d473..6761138f6 100644 --- a/packages/core/src/lib/history.ts +++ b/packages/core/src/lib/history.ts @@ -13,9 +13,7 @@ export type HistoryOnlyOptions = { skipUploads?: boolean; forceCleanStatus?: boolean; }; -export type HistoryOptions = Required< - Pick & Required> -> & { +export type HistoryOptions = Pick & { persist: Required; upload?: Required; } & HistoryOnlyOptions & diff --git a/packages/core/src/lib/history.unit.test.ts b/packages/core/src/lib/history.unit.test.ts index 5f24142a0..85cfbf5e1 100644 --- a/packages/core/src/lib/history.unit.test.ts +++ b/packages/core/src/lib/history.unit.test.ts @@ -30,7 +30,6 @@ describe('history', () => { format: ['json'], }, plugins: [MINIMAL_PLUGIN_CONFIG_MOCK], - categories: [], }; it('should check out all passed commits and reset to initial branch or tag', async () => { await history(historyBaseOptions, ['abc', 'def']); diff --git a/packages/core/src/lib/implementation/collect.integration.test.ts b/packages/core/src/lib/implementation/collect.integration.test.ts index a0437f16f..8de4662ae 100644 --- a/packages/core/src/lib/implementation/collect.integration.test.ts +++ b/packages/core/src/lib/implementation/collect.integration.test.ts @@ -8,7 +8,6 @@ describe('collect', () => { it('should execute with valid options', async () => { vol.fromJSON({}, MEMFS_VOLUME); const report = await collect({ - categories: [], ...MINIMAL_CONFIG_MOCK, verbose: true, progress: false, diff --git a/packages/core/src/lib/implementation/collect.ts b/packages/core/src/lib/implementation/collect.ts index acc32d827..783437a57 100644 --- a/packages/core/src/lib/implementation/collect.ts +++ b/packages/core/src/lib/implementation/collect.ts @@ -4,9 +4,7 @@ import { name, version } from '../../../package.json'; import type { GlobalOptions } from '../types'; import { executePlugins } from './execute-plugin'; -export type CollectOptions = Required< - Pick -> & +export type CollectOptions = Pick & Partial; /** diff --git a/packages/core/src/lib/implementation/compare-scorables.ts b/packages/core/src/lib/implementation/compare-scorables.ts index 97f20be30..f2f44c798 100644 --- a/packages/core/src/lib/implementation/compare-scorables.ts +++ b/packages/core/src/lib/implementation/compare-scorables.ts @@ -26,8 +26,8 @@ export function compareCategories( reports: ReportsToCompare, ): ReportsDiff['categories'] { const { pairs, added, removed } = matchArrayItemsByKey({ - before: reports.before.categories, - after: reports.after.categories, + before: reports.before.categories ?? [], + after: reports.after.categories ?? [], key: 'slug', }); const { changed, unchanged } = comparePairs( diff --git a/packages/core/src/lib/implementation/report-to-gql.ts b/packages/core/src/lib/implementation/report-to-gql.ts index b6ddab1ff..e5538aa74 100644 --- a/packages/core/src/lib/implementation/report-to-gql.ts +++ b/packages/core/src/lib/implementation/report-to-gql.ts @@ -35,7 +35,7 @@ export function reportToGQL( commandStartDate: report.date, commandDuration: report.duration, plugins: report.plugins.map(pluginToGQL), - categories: report.categories.map(categoryToGQL), + categories: (report.categories ?? []).map(categoryToGQL), }; } diff --git a/packages/models/src/lib/core-config.ts b/packages/models/src/lib/core-config.ts index bf402f0e7..a6bffc39d 100644 --- a/packages/models/src/lib/core-config.ts +++ b/packages/models/src/lib/core-config.ts @@ -32,13 +32,10 @@ export const coreConfigSchema = refineCoreConfig(unrefinedCoreConfigSchema); export function refineCoreConfig(schema: typeof unrefinedCoreConfigSchema) { // categories point to existing audit or group refs return schema.refine( - coreCfg => - !getMissingRefsForCategories(coreCfg.categories ?? [], coreCfg.plugins), - coreCfg => ({ - message: missingRefsForCategoriesErrorMsg( - coreCfg.categories ?? [], - coreCfg.plugins, - ), + ({ plugins, categories }) => + !getMissingRefsForCategories(plugins, categories), + ({ plugins, categories }) => ({ + message: missingRefsForCategoriesErrorMsg(plugins, categories), }), ) as unknown as typeof unrefinedCoreConfigSchema; } diff --git a/packages/models/src/lib/implementation/utils.ts b/packages/models/src/lib/implementation/utils.ts index 5ad600434..4851c1f69 100644 --- a/packages/models/src/lib/implementation/utils.ts +++ b/packages/models/src/lib/implementation/utils.ts @@ -64,10 +64,10 @@ export function exists(value: T): value is NonNullable { * @returns Array of missing references. */ export function getMissingRefsForCategories( - categories: CategoryConfig[], plugins: PluginConfig[] | PluginReport[], + categories?: CategoryConfig[], ) { - if (categories.length === 0) { + if (!categories || categories.length === 0) { return false; } @@ -108,10 +108,10 @@ export function getMissingRefsForCategories( } export function missingRefsForCategoriesErrorMsg( - categories: CategoryConfig[], plugins: PluginConfig[] | PluginReport[], + categories?: CategoryConfig[], ) { - const missingRefs = getMissingRefsForCategories(categories, plugins); + const missingRefs = getMissingRefsForCategories(plugins, categories); return `The following category references need to point to an audit or group: ${errorItems( missingRefs, )}`; diff --git a/packages/models/src/lib/report.ts b/packages/models/src/lib/report.ts index 166836147..920a5097a 100644 --- a/packages/models/src/lib/report.ts +++ b/packages/models/src/lib/report.ts @@ -75,8 +75,8 @@ export const reportSchema = packageVersionSchema({ .merge( z.object( { - categories: z.array(categoryConfigSchema), plugins: z.array(pluginReportSchema).min(1), + categories: z.array(categoryConfigSchema).optional(), commit: commitSchema .describe('Git commit for which report was collected') .nullable(), @@ -85,12 +85,10 @@ export const reportSchema = packageVersionSchema({ ), ) .refine( - report => !getMissingRefsForCategories(report.categories, report.plugins), - report => ({ - message: missingRefsForCategoriesErrorMsg( - report.categories, - report.plugins, - ), + ({ plugins, categories }) => + !getMissingRefsForCategories(plugins, categories), + ({ plugins, categories }) => ({ + message: missingRefsForCategoriesErrorMsg(plugins, categories), }), ); diff --git a/packages/models/src/lib/report.unit.test.ts b/packages/models/src/lib/report.unit.test.ts index 4a3ea454f..63a5d18ba 100644 --- a/packages/models/src/lib/report.unit.test.ts +++ b/packages/models/src/lib/report.unit.test.ts @@ -219,7 +219,6 @@ describe('reportSchema', () => { packageName: 'cli', version: '1.0.1', plugins: [], - categories: [], }), ).toThrow('too_small'); }); diff --git a/packages/utils/perf/score-report/optimized0.ts b/packages/utils/perf/score-report/optimized0.ts index 540f812ca..2b391443f 100644 --- a/packages/utils/perf/score-report/optimized0.ts +++ b/packages/utils/perf/score-report/optimized0.ts @@ -78,7 +78,7 @@ export function scoreReportOptimized0(report: Report): ScoredReport { const allScoredAudits = scoredPlugins.flatMap(({ audits }) => audits); const allScoredGroups = scoredPlugins.flatMap(({ groups }) => groups); - const scoredCategories = report.categories.map(category => ({ + const scoredCategories = report.categories?.map(category => ({ ...category, score: calculateScore( category.refs, diff --git a/packages/utils/src/lib/reports/generate-md-report-categoy-section.ts b/packages/utils/src/lib/reports/generate-md-report-categoy-section.ts index 232c5b97d..3f7fdada0 100644 --- a/packages/utils/src/lib/reports/generate-md-report-categoy-section.ts +++ b/packages/utils/src/lib/reports/generate-md-report-categoy-section.ts @@ -14,7 +14,7 @@ import { } from './utils'; export function categoriesOverviewSection( - report: Pick, + report: Required>, ): MarkdownDocument { const { categories, plugins } = report; return new MarkdownDocument().table( @@ -36,7 +36,7 @@ export function categoriesOverviewSection( } export function categoriesDetailsSection( - report: Pick, + report: Required>, ): MarkdownDocument { const { categories, plugins } = report; diff --git a/packages/utils/src/lib/reports/generate-md-report.ts b/packages/utils/src/lib/reports/generate-md-report.ts index e03b29d57..82aae4264 100644 --- a/packages/utils/src/lib/reports/generate-md-report.ts +++ b/packages/utils/src/lib/reports/generate-md-report.ts @@ -13,7 +13,11 @@ import { categoriesDetailsSection, categoriesOverviewSection, } from './generate-md-report-categoy-section'; -import type { MdReportOptions, ScoredReport } from './types'; +import type { + MdReportOptions, + ScoredCategoryConfig, + ScoredReport, +} from './types'; import { formatReportScore, scoreMarker, severityMarker } from './utils'; export function auditDetailsAuditValue({ @@ -26,19 +30,25 @@ export function auditDetailsAuditValue({ )} (score: ${formatReportScore(score)})`; } +function hasCategories( + report: ScoredReport, +): report is ScoredReport & { categories: ScoredCategoryConfig[] } { + return !!report.categories && report.categories.length > 0; +} + export function generateMdReport( report: ScoredReport, options?: MdReportOptions, ): string { return new MarkdownDocument() .heading(HIERARCHY.level_1, REPORT_HEADLINE_TEXT) - .$if(report.categories.length > 0, doc => - doc.$concat( - categoriesOverviewSection(report), - categoriesDetailsSection(report), - ), + .$concat( + ...(hasCategories(report) + ? [categoriesOverviewSection(report), categoriesDetailsSection(report)] + : []), + auditsSection(report, options), + aboutSection(report), ) - .$concat(auditsSection(report, options), aboutSection(report)) .rule() .paragraph(md`${FOOTER_PREFIX} ${md.link(README_LINK, 'Code PushUp')}`) .toString(); @@ -179,7 +189,7 @@ export function reportMetaTable({ md.code(version), formatDuration(duration), plugins.length.toString(), - categories.length.toString(), + (categories?.length ?? 0).toString(), plugins.reduce((acc, { audits }) => acc + audits.length, 0).toString(), ], ], diff --git a/packages/utils/src/lib/reports/generate-md-report.unit.test.ts b/packages/utils/src/lib/reports/generate-md-report.unit.test.ts index 488c24943..0b69f08ab 100644 --- a/packages/utils/src/lib/reports/generate-md-report.unit.test.ts +++ b/packages/utils/src/lib/reports/generate-md-report.unit.test.ts @@ -551,8 +551,8 @@ describe('generateMdReport', () => { expect(md).toMatch('Made with ❤ by [Code PushUp]'); }); - it('should skip categories section if empty', () => { - const md = generateMdReport({ ...baseScoredReport, categories: [] }); + it('should skip categories section when categories are missing', () => { + const md = generateMdReport({ ...baseScoredReport, categories: undefined }); expect(md).not.toMatch('## 🏷 Categories'); expect(md).toMatch('# Code PushUp Report\n\n## 🛡️ Audits'); }); diff --git a/packages/utils/src/lib/reports/log-stdout-summary.integration.test.ts b/packages/utils/src/lib/reports/log-stdout-summary.integration.test.ts index 072e0b274..2b3a3b4a8 100644 --- a/packages/utils/src/lib/reports/log-stdout-summary.integration.test.ts +++ b/packages/utils/src/lib/reports/log-stdout-summary.integration.test.ts @@ -37,9 +37,9 @@ describe('logStdoutSummary', () => { ); }); - it('should not contain category section when categories are empty', async () => { + it('should not contain category section when categories are missing', async () => { logStdoutSummary( - sortReport(scoreReport({ ...reportMock(), categories: [] })), + sortReport(scoreReport({ ...reportMock(), categories: undefined })), ); const output = logs.join('\n'); diff --git a/packages/utils/src/lib/reports/log-stdout-summary.ts b/packages/utils/src/lib/reports/log-stdout-summary.ts index e6a397f2c..59a8c5716 100644 --- a/packages/utils/src/lib/reports/log-stdout-summary.ts +++ b/packages/utils/src/lib/reports/log-stdout-summary.ts @@ -16,20 +16,21 @@ function log(msg = ''): void { } export function logStdoutSummary(report: ScoredReport, verbose = false): void { - const printCategories = report.categories.length > 0; - - log(reportToHeaderSection(report)); + const { plugins, categories, packageName, version } = report; + log(reportToHeaderSection({ packageName, version })); log(); - logPlugins(report.plugins, verbose); - if (printCategories) { - logCategories(report); + logPlugins(plugins, verbose); + if (categories && categories.length > 0) { + logCategories({ plugins, categories }); } log(`${FOOTER_PREFIX} ${CODE_PUSHUP_DOMAIN}`); log(); } -function reportToHeaderSection(report: ScoredReport): string { - const { packageName, version } = report; +function reportToHeaderSection({ + packageName, + version, +}: Pick): string { return `${bold(REPORT_HEADLINE_TEXT)} - ${packageName}@${version}`; } @@ -92,7 +93,10 @@ function logRow(score: number, title: string, value?: string): void { ]); } -export function logCategories({ categories, plugins }: ScoredReport): void { +export function logCategories({ + plugins, + categories, +}: Required>): void { const hAlign = (idx: number) => (idx === 0 ? 'left' : 'right'); const rows = categories.map(({ title, score, refs, isBinary }) => [ diff --git a/packages/utils/src/lib/reports/log-stdout-summary.unit.test.ts b/packages/utils/src/lib/reports/log-stdout-summary.unit.test.ts index fbcc85879..0bd287b01 100644 --- a/packages/utils/src/lib/reports/log-stdout-summary.unit.test.ts +++ b/packages/utils/src/lib/reports/log-stdout-summary.unit.test.ts @@ -61,7 +61,7 @@ describe('logCategories', () => { }, ] as ScoredReport['plugins']; - logCategories({ categories, plugins } as ScoredReport); + logCategories({ plugins, categories }); const output = logs.join('\n'); @@ -105,7 +105,7 @@ describe('logCategories', () => { }, ] as ScoredReport['plugins']; - logCategories({ categories, plugins } as ScoredReport); + logCategories({ plugins, categories }); const output = logs.join('\n'); @@ -149,7 +149,7 @@ describe('logCategories', () => { }, ] as ScoredReport['plugins']; - logCategories({ categories, plugins } as ScoredReport); + logCategories({ plugins, categories }); const output = logs.join('\n'); diff --git a/packages/utils/src/lib/reports/scoring.ts b/packages/utils/src/lib/reports/scoring.ts index 92369e5a2..0bdea95d1 100644 --- a/packages/utils/src/lib/reports/scoring.ts +++ b/packages/utils/src/lib/reports/scoring.ts @@ -62,7 +62,7 @@ export function scoreReport(report: Report): ScoredReport { return item.score; } - const scoredCategories = report.categories.map(category => ({ + const scoredCategories = report.categories?.map(category => ({ ...category, score: calculateScore(category.refs, catScoreFn), })); diff --git a/packages/utils/src/lib/reports/scoring.unit.test.ts b/packages/utils/src/lib/reports/scoring.unit.test.ts index c5ce46ce9..cb0ab3294 100644 --- a/packages/utils/src/lib/reports/scoring.unit.test.ts +++ b/packages/utils/src/lib/reports/scoring.unit.test.ts @@ -130,9 +130,9 @@ describe('scoreReport', () => { ); }); - it('should accept a report with empty categories', () => { - expect(scoreReport({ ...REPORT_MOCK, categories: [] })).toEqual( - expect.objectContaining({ categories: [] }), + it('should accept a report with no categories', () => { + expect(scoreReport({ ...REPORT_MOCK, categories: undefined })).toEqual( + expect.objectContaining({ categories: undefined }), ); }); }); diff --git a/packages/utils/src/lib/reports/sorting.integration.test.ts b/packages/utils/src/lib/reports/sorting.integration.test.ts index 1cf763bd3..4f22f7f4d 100644 --- a/packages/utils/src/lib/reports/sorting.integration.test.ts +++ b/packages/utils/src/lib/reports/sorting.integration.test.ts @@ -57,8 +57,10 @@ describe('sortReport', () => { it('should sort a report with no categories', () => { const sortedReport = sortReport( - scoreReport({ ...REPORT_MOCK, categories: [] }), + scoreReport({ ...REPORT_MOCK, categories: undefined }), + ); + expect(sortedReport).toEqual( + expect.objectContaining({ categories: undefined }), ); - expect(sortedReport).toEqual(expect.objectContaining({ categories: [] })); }); }); diff --git a/packages/utils/src/lib/reports/sorting.ts b/packages/utils/src/lib/reports/sorting.ts index 35efd30c1..e922e10fa 100644 --- a/packages/utils/src/lib/reports/sorting.ts +++ b/packages/utils/src/lib/reports/sorting.ts @@ -91,7 +91,7 @@ export function getSortableGroupByRef( export function sortReport(report: ScoredReport): ScoredReport { const { categories, plugins } = report; - const sortedCategories = categories.map(category => { + const sortedCategories = categories?.map(category => { const { audits, groups } = category.refs.reduce( ( acc: { diff --git a/packages/utils/src/lib/reports/types.ts b/packages/utils/src/lib/reports/types.ts index 941b906ee..1c89d5986 100644 --- a/packages/utils/src/lib/reports/types.ts +++ b/packages/utils/src/lib/reports/types.ts @@ -17,7 +17,7 @@ export type ScoredReport = Omit & { plugins: (Omit & { groups?: ScoredGroup[]; })[]; - categories: ScoredCategoryConfig[]; + categories?: ScoredCategoryConfig[]; }; export type SortableGroup = ScoredGroup & { diff --git a/testing/test-utils/src/lib/utils/report.mock.ts b/testing/test-utils/src/lib/utils/report.mock.ts index 14fad148b..bad186816 100644 --- a/testing/test-utils/src/lib/utils/report.mock.ts +++ b/testing/test-utils/src/lib/utils/report.mock.ts @@ -11,7 +11,6 @@ export const MINIMAL_REPORT_MOCK: Report = { date: '2023-08-16T09:00:00.000Z', duration: 666, commit: COMMIT_MOCK, - categories: [], plugins: [ { slug: 'eslint', From 9a744b8201fd7a0aa1114d70c1b32abc5e3d4216 Mon Sep 17 00:00:00 2001 From: hanna-skryl Date: Thu, 7 Nov 2024 13:43:36 -0500 Subject: [PATCH 2/4] refactor(ci): handle undefined categories --- packages/ci/src/lib/issues.ts | 32 +++++++--- packages/ci/src/lib/issues.unit.test.ts | 83 ++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 12 deletions(-) diff --git a/packages/ci/src/lib/issues.ts b/packages/ci/src/lib/issues.ts index 526a30469..e1fd6775d 100644 --- a/packages/ci/src/lib/issues.ts +++ b/packages/ci/src/lib/issues.ts @@ -1,6 +1,7 @@ import type { Audit, AuditReport, + CategoryRef, Issue, PluginMeta, Report, @@ -151,6 +152,9 @@ export function getAuditImpactValue( { audit, plugin }: IssueContext, report: Report, ): number { + if (!report.categories) { + return 0; + } return report.categories .map((category): number => { const weights = category.refs.map((ref): number => { @@ -163,16 +167,7 @@ export function getAuditImpactValue( return ref.slug === audit.slug ? ref.weight : 0; case 'group': - const group = report.plugins - .find(({ slug }) => slug === ref.plugin) - ?.groups?.find(({ slug }) => slug === ref.slug); - if (!group?.refs.length) { - return 0; - } - const groupRatio = - (group.refs.find(({ slug }) => slug === audit.slug)?.weight ?? - 0) / group.refs.reduce((acc, { weight }) => acc + weight, 0); - return ref.weight * groupRatio; + return calculateGroupImpact(ref, audit, report); } }); @@ -183,3 +178,20 @@ export function getAuditImpactValue( }) .reduce((acc, value) => acc + value, 0); } + +export function calculateGroupImpact( + ref: CategoryRef, + audit: Audit, + report: Report, +): number { + const group = report.plugins + .find(({ slug }) => slug === ref.plugin) + ?.groups?.find(({ slug }) => slug === ref.slug); + if (!group?.refs.length) { + return 0; + } + const groupRatio = + (group.refs.find(({ slug }) => slug === audit.slug)?.weight ?? 0) / + group.refs.reduce((acc, { weight }) => acc + weight, 0); + return ref.weight * groupRatio; +} diff --git a/packages/ci/src/lib/issues.unit.test.ts b/packages/ci/src/lib/issues.unit.test.ts index 7de57f999..5e61da97f 100644 --- a/packages/ci/src/lib/issues.unit.test.ts +++ b/packages/ci/src/lib/issues.unit.test.ts @@ -1,5 +1,9 @@ -import type { Report } from '@code-pushup/models'; -import { getAuditImpactValue, issuesMatch } from './issues'; +import type { CategoryRef, Report } from '@code-pushup/models'; +import { + calculateGroupImpact, + getAuditImpactValue, + issuesMatch, +} from './issues'; describe('issues comparison', () => { it('should match issues with exact same metadata', () => { @@ -271,4 +275,79 @@ describe('issues sorting', () => { ), ).toBe(0.09); // 1% + 8% = 9% }); + + it('should return 0 when there are no categories', () => { + expect( + getAuditImpactValue( + { + audit: { + slug: 'react-jsx-key', + title: 'Disallow missing `key` props in iterators', + }, + plugin: { slug: 'eslint', title: 'ESLint' }, + }, + { + plugins: [ + { + slug: 'eslint', + groups: [ + { + slug: 'suggestions', + refs: [{ slug: 'mock-rule', weight: 1 }], + }, + ], + }, + ], + } as Report, + ), + ).toBe(0); + }); +}); + +describe('calculateGroupImpact', () => { + const mockAudit = { + slug: 'react-jsx-key', + title: 'Disallow missing `key` props in iterators', + }; + const mockCategoryRef = { + type: 'group', + plugin: 'eslint', + slug: 'suggestions', + weight: 1, + } as CategoryRef; + + const mockReport = { + plugins: [ + { + slug: 'eslint', + groups: [ + { + slug: 'suggestions', + refs: [ + ...Array.from({ length: 9 }).map((_, i) => ({ + slug: `mock-rule-${i}`, + weight: 1, + })), + { slug: 'react-jsx-key', weight: 1 }, + ], + }, + ], + }, + ], + } as Report; + + it('should calculate correct impact for audit in group', () => { + expect(calculateGroupImpact(mockCategoryRef, mockAudit, mockReport)).toBe( + 0.1, + ); + }); + + it('should return 0 if the group has no refs', () => { + const emptyGroupRef = { + ...mockCategoryRef, + plugin: 'eslint', + slug: 'nonexistent-group', + }; + expect(calculateGroupImpact(emptyGroupRef, mockAudit, mockReport)).toBe(0); + }); }); From b1c1fe4a2cf8cf1909c289c55d8634318f2f7415 Mon Sep 17 00:00:00 2001 From: hanna-skryl Date: Thu, 7 Nov 2024 14:23:11 -0500 Subject: [PATCH 3/4] test(utils): correct type cast --- .../generate-md-report-category-section.unit.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/utils/src/lib/reports/generate-md-report-category-section.unit.test.ts b/packages/utils/src/lib/reports/generate-md-report-category-section.unit.test.ts index 26c09535b..a855f6c16 100644 --- a/packages/utils/src/lib/reports/generate-md-report-category-section.unit.test.ts +++ b/packages/utils/src/lib/reports/generate-md-report-category-section.unit.test.ts @@ -45,7 +45,7 @@ describe('categoriesOverviewSection', () => { refs: [{ slug: 'no-any', type: 'audit' }], }, ], - } as ScoredReport).toString(), + } as Required>).toString(), ).toMatchSnapshot(); }); @@ -67,7 +67,7 @@ describe('categoriesOverviewSection', () => { refs: [{ slug: 'no-let', type: 'audit' }], }, ], - } as ScoredReport).toString(), + } as Required>).toString(), ).toMatchSnapshot(); }); }); @@ -211,7 +211,7 @@ describe('categoriesDetailsSection', () => { refs: [{ slug: 'no-any', type: 'audit', plugin: 'eslint' }], }, ], - } as ScoredReport).toString(), + } as Required>).toString(), ).toMatchSnapshot(); }); @@ -234,7 +234,7 @@ describe('categoriesDetailsSection', () => { refs: [{ slug: 'no-let', type: 'audit', plugin: 'eslint' }], }, ], - } as ScoredReport).toString(), + } as Required>).toString(), ).toMatchSnapshot(); }); @@ -257,7 +257,7 @@ describe('categoriesDetailsSection', () => { refs: [{ slug: 'no-let', type: 'audit', plugin: 'eslint' }], }, ], - } as ScoredReport).toString(), + } as Required>).toString(), ).toMatchSnapshot(); }); }); From d2c9bbc96944abd562df42802bde70c42e2d4d0f Mon Sep 17 00:00:00 2001 From: hanna-skryl Date: Fri, 8 Nov 2024 09:37:33 -0500 Subject: [PATCH 4/4] refactor: standardize undefined handling for categories --- packages/ci/src/lib/issues.unit.test.ts | 9 --------- packages/models/src/lib/core-config.ts | 8 ++++---- packages/models/src/lib/implementation/utils.ts | 6 +++--- packages/models/src/lib/report.ts | 8 ++++---- packages/utils/src/lib/reports/generate-md-report.ts | 8 ++------ 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/packages/ci/src/lib/issues.unit.test.ts b/packages/ci/src/lib/issues.unit.test.ts index 5e61da97f..5fd854285 100644 --- a/packages/ci/src/lib/issues.unit.test.ts +++ b/packages/ci/src/lib/issues.unit.test.ts @@ -341,13 +341,4 @@ describe('calculateGroupImpact', () => { 0.1, ); }); - - it('should return 0 if the group has no refs', () => { - const emptyGroupRef = { - ...mockCategoryRef, - plugin: 'eslint', - slug: 'nonexistent-group', - }; - expect(calculateGroupImpact(emptyGroupRef, mockAudit, mockReport)).toBe(0); - }); }); diff --git a/packages/models/src/lib/core-config.ts b/packages/models/src/lib/core-config.ts index a6bffc39d..711825841 100644 --- a/packages/models/src/lib/core-config.ts +++ b/packages/models/src/lib/core-config.ts @@ -32,10 +32,10 @@ export const coreConfigSchema = refineCoreConfig(unrefinedCoreConfigSchema); export function refineCoreConfig(schema: typeof unrefinedCoreConfigSchema) { // categories point to existing audit or group refs return schema.refine( - ({ plugins, categories }) => - !getMissingRefsForCategories(plugins, categories), - ({ plugins, categories }) => ({ - message: missingRefsForCategoriesErrorMsg(plugins, categories), + ({ categories, plugins }) => + !getMissingRefsForCategories(categories, plugins), + ({ categories, plugins }) => ({ + message: missingRefsForCategoriesErrorMsg(categories, plugins), }), ) as unknown as typeof unrefinedCoreConfigSchema; } diff --git a/packages/models/src/lib/implementation/utils.ts b/packages/models/src/lib/implementation/utils.ts index 4851c1f69..f99a4d593 100644 --- a/packages/models/src/lib/implementation/utils.ts +++ b/packages/models/src/lib/implementation/utils.ts @@ -64,8 +64,8 @@ export function exists(value: T): value is NonNullable { * @returns Array of missing references. */ export function getMissingRefsForCategories( + categories: CategoryConfig[] | undefined, plugins: PluginConfig[] | PluginReport[], - categories?: CategoryConfig[], ) { if (!categories || categories.length === 0) { return false; @@ -108,10 +108,10 @@ export function getMissingRefsForCategories( } export function missingRefsForCategoriesErrorMsg( + categories: CategoryConfig[] | undefined, plugins: PluginConfig[] | PluginReport[], - categories?: CategoryConfig[], ) { - const missingRefs = getMissingRefsForCategories(plugins, categories); + const missingRefs = getMissingRefsForCategories(categories, plugins); return `The following category references need to point to an audit or group: ${errorItems( missingRefs, )}`; diff --git a/packages/models/src/lib/report.ts b/packages/models/src/lib/report.ts index 920a5097a..ea3b11c88 100644 --- a/packages/models/src/lib/report.ts +++ b/packages/models/src/lib/report.ts @@ -85,10 +85,10 @@ export const reportSchema = packageVersionSchema({ ), ) .refine( - ({ plugins, categories }) => - !getMissingRefsForCategories(plugins, categories), - ({ plugins, categories }) => ({ - message: missingRefsForCategoriesErrorMsg(plugins, categories), + ({ categories, plugins }) => + !getMissingRefsForCategories(categories, plugins), + ({ categories, plugins }) => ({ + message: missingRefsForCategoriesErrorMsg(categories, plugins), }), ); diff --git a/packages/utils/src/lib/reports/generate-md-report.ts b/packages/utils/src/lib/reports/generate-md-report.ts index 82aae4264..13a0e71f6 100644 --- a/packages/utils/src/lib/reports/generate-md-report.ts +++ b/packages/utils/src/lib/reports/generate-md-report.ts @@ -13,11 +13,7 @@ import { categoriesDetailsSection, categoriesOverviewSection, } from './generate-md-report-categoy-section'; -import type { - MdReportOptions, - ScoredCategoryConfig, - ScoredReport, -} from './types'; +import type { MdReportOptions, ScoredReport } from './types'; import { formatReportScore, scoreMarker, severityMarker } from './utils'; export function auditDetailsAuditValue({ @@ -32,7 +28,7 @@ export function auditDetailsAuditValue({ function hasCategories( report: ScoredReport, -): report is ScoredReport & { categories: ScoredCategoryConfig[] } { +): report is ScoredReport & Required> { return !!report.categories && report.categories.length > 0; }