Skip to content

Commit

Permalink
refactor: standardize undefined handling for categories
Browse files Browse the repository at this point in the history
  • Loading branch information
hanna-skryl authored Nov 8, 2024
1 parent 01724d3 commit 7e6c29d
Show file tree
Hide file tree
Showing 37 changed files with 211 additions and 123 deletions.
32 changes: 22 additions & 10 deletions packages/ci/src/lib/issues.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type {
Audit,
AuditReport,
CategoryRef,
Issue,
PluginMeta,
Report,
Expand Down Expand Up @@ -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 => {
Expand All @@ -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);
}
});

Expand All @@ -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;
}
74 changes: 72 additions & 2 deletions packages/ci/src/lib/issues.unit.test.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -271,4 +275,70 @@ 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,
);
});
});
2 changes: 1 addition & 1 deletion packages/cli/src/lib/autorun/autorun-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export function yargsAutorunCommandObject() {
await collectAndPersistReports(optionsWithFormat);
collectSuccessfulLog();

if (options.categories.length === 0) {
if (!options.categories || options.categories.length === 0) {
renderConfigureCategoriesHint();
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/lib/collect/collect-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<CoreConfig>(
['--config=./no-category.config.ts'],
{
Expand All @@ -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 () => {
Expand Down
2 changes: 0 additions & 2 deletions packages/cli/src/lib/implementation/core-config.middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ export async function coreConfigMiddleware<
const {
persist: rcPersist,
upload: rcUpload,
categories: rcCategories,
...remainingRcConfig
} = importedRc;
const upload =
Expand All @@ -56,7 +55,6 @@ export async function coreConfigMiddleware<
),
},
...(upload != null && { upload }),
categories: rcCategories ?? [],
...remainingRcConfig,
...remainingCliOptions,
};
Expand Down
33 changes: 20 additions & 13 deletions packages/cli/src/lib/implementation/filter.middleware.ts
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -12,7 +12,7 @@ export function filterMiddleware<T extends FilterOptions>(
): T {
const {
plugins,
categories = [],
categories,
skipCategories = [],
onlyCategories = [],
skipPlugins = [],
Expand All @@ -26,7 +26,7 @@ export function filterMiddleware<T extends FilterOptions>(
skipPlugins.length === 0 &&
onlyPlugins.length === 0
) {
return { ...originalProcessArgs, categories };
return originalProcessArgs;
}

handleConflictingOptions('categories', onlyCategories, skipCategories);
Expand All @@ -44,9 +44,11 @@ export function filterMiddleware<T extends FilterOptions>(
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 },
Expand Down Expand Up @@ -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(
Expand All @@ -102,7 +108,7 @@ function applyPluginFilters(
skipPlugins: string[],
onlyPlugins: string[],
verbose: boolean,
): PluginConfig[] {
): CoreConfig['plugins'] {
const filteredPlugins = filterPluginsFromCategories({
categories,
plugins,
Expand All @@ -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));
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ describe('filterMiddleware', () => {
}),
).toStrictEqual({
plugins: [{ slug: 'p1', audits: [{ slug: 'a1-p1' }] }],
categories: [],
});
});

Expand Down Expand Up @@ -89,7 +88,6 @@ describe('filterMiddleware', () => {
const { plugins } = filterMiddleware({
...option,
plugins: [{ slug: 'p1' }, { slug: 'p2' }] as PluginConfig[],
categories: [],
});
expect(plugins).toStrictEqual([expect.objectContaining(expected)]);
},
Expand Down
11 changes: 2 additions & 9 deletions packages/cli/src/lib/implementation/filter.model.ts
Original file line number Diff line number Diff line change
@@ -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<GlobalOptions> &
Pick<CoreConfig, 'categories' | 'plugins'> &
Expand All @@ -17,7 +13,4 @@ export type FilterOptionType =
| 'skipPlugins'
| 'onlyPlugins';

export type Filterables = {
categories: CategoryConfig[];
plugins: PluginConfig[];
};
export type Filterables = Pick<CoreConfig, 'plugins' | 'categories'>;
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
);
Expand Down Expand Up @@ -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 },
);
Expand All @@ -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(
Expand Down Expand Up @@ -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', () => {
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/lib/collect-and-persist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<CoreConfig, 'plugins' | 'categories'>
export type CollectAndPersistReportsOptions = Pick<
CoreConfig,
'plugins' | 'categories'
> & { persist: Required<PersistConfig> } & Partial<GlobalOptions>;

export async function collectAndPersistReports(
Expand Down
Loading

0 comments on commit 7e6c29d

Please sign in to comment.