Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: standardize undefined handling for categories #868

Merged
merged 4 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
{ 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 @@
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 @@
})
.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) {

Check failure on line 190 in packages/ci/src/lib/issues.ts

View workflow job for this annotation

GitHub Actions / Code PushUp

<✓> Code coverage | Branch coverage

1st branch is not taken in any test case.
return 0;
}

Check warning on line 192 in packages/ci/src/lib/issues.ts

View workflow job for this annotation

GitHub Actions / Code PushUp

<✓> Code coverage | Line coverage

Lines 191-192 are not covered in any test case.
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