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.

Sign up for GitHub

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

fix(#2553): remove config with no properties + support metadata check for config without properties #2641

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(#2553): remove config with no properties + support metadata check for config without properties #2641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .yarnrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ packageExtensions:
"@swc/types": "*"
"@typescript-eslint/rule-tester@*":
dependencies:
"@typescript-eslint/parser": ~8.17.0
"@typescript-eslint/parser": ~8.18.0
"@angular-eslint/eslint-plugin-template@*":
dependencies:
"@typescript-eslint/types": "^8.0.0"
Expand Down
2 changes: 1 addition & 1 deletion packages/@ama-sdk/schematics/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@
"@swc/helpers": "~0.5.0",
"@commitlint/cli": "^19.0.0",
"@commitlint/config-conventional": "^19.0.0",
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"jest-junit": "~16.0.0",
"lint-staged": "^15.0.0",
"minimist": "^1.2.6",
Expand Down
3 changes: 3 additions & 0 deletions ...ges/@o3r/components/builders/component-extractor/helpers/component/component.extractor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,9 @@
} else {
this.logger.warn(message);
}
if (propertiesWithDefaultValue.length === 0) {
return acc;

Check warning on line 280 in packages/@o3r/components/builders/component-extractor/helpers/component/component.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/components/builders/component-extractor/helpers/component/component.extractor.ts#L280 

Added line #L280 was not covered by tests
}
const configWithoutIncompatibleProperties = {
...config,
properties: propertiesWithDefaultValue
Expand Down
94 changes: 94 additions & 0 deletions ...@o3r/components/builders/metadata-check/helpers/config-metadata-comparison.helper.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import {
configMetadataComparator,
} from './config-metadata-comparison.helper';
import {
ComponentConfigOutput,
} from '@o3r/components';

describe('configMetadataComparator', () => {
describe('getArray', () => {
it('should return one element per configuration property', () => {
const result = configMetadataComparator.getArray([
{ properties: [
{ description: 'description', label: 'prop1', name: 'prop1', type: 'string' },
{ description: 'description', label: 'prop2', name: 'prop2', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT' }
]);
expect(result.length).toBe(2);
result.forEach((item) => {
expect(item.properties.length).toBe(1);
});
});

it('should return only configurations with properties', () => {
const result = configMetadataComparator.getArray([
{ properties: [], library: 'lib', name: 'ConfigWithoutProp', path: '', type: 'COMPONENT' },
{ properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT' }
]);
expect(result.length).toBe(1);
expect(result[0].name).toBe('ConfigWithProp');
});
});

describe('getIdentifier', () => {
it('should return an identifier with the property name', () => {
const result = configMetadataComparator.getIdentifier({
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
});
expect(result).toBe('lib#ConfigWithProp prop');
});

it('should return an identifier without property name if none available', () => {
const result = configMetadataComparator.getIdentifier({
properties: [], library: 'lib', name: 'ConfigWithoutProp', path: '', type: 'COMPONENT'
});
expect(result).toBe('lib#ConfigWithoutProp');
});
});

describe('isMigrationDataMatch', () => {
it('should return true', () => {
const metadata = {
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
} satisfies ComponentConfigOutput;
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp', propertyName: 'prop' }
)).toBeTruthy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp' }
)).toBeTruthy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib' }
)).toBeTruthy();
});

it('should return false', () => {
const metadata = {
properties: [
{ description: 'description', label: 'prop', name: 'prop', type: 'string' }
], library: 'lib', name: 'ConfigWithProp', path: '', type: 'COMPONENT'
} satisfies ComponentConfigOutput;
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'anotherLib', configName: 'ConfigWithProp', propertyName: 'prop' }
)).toBeFalsy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'AnotherConfig', propertyName: 'prop' }
)).toBeFalsy();
expect(configMetadataComparator.isMigrationDataMatch(
metadata,
{ libraryName: 'lib', configName: 'ConfigWithProp', propertyName: 'anotherProp' }
)).toBeFalsy();
});
});
});
4 changes: 1 addition & 3 deletions ...ages/@o3r/components/builders/metadata-check/helpers/config-metadata-comparison.helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export interface MigrationConfigData {
* ```
*/
const getConfigurationArray = (content: ComponentConfigOutput[]): ComponentConfigOutput[] => content.flatMap((config) =>
config.properties.length > 1
? config.properties.map((prop) => ({ ...config, properties: [prop] }))
: [config]
config.properties.map((prop) => ({ ...config, properties: [prop] }))
);

const getConfigurationPropertyName = (config: ComponentConfigOutput) => `${config.library}#${config.name}` + (config.properties.length > 0 ? ` ${config.properties[0].name}` : '');
Expand Down
73 changes: 54 additions & 19 deletions packages/@o3r/components/builders/metadata-check/index.it.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ import {
import {
inc,
} from 'semver';
import type {
MigrationConfigData,
import {
configMetadataComparator,
type MigrationConfigData,
} from './helpers/config-metadata-comparison.helper';
import type {
ComponentConfigOutput,
Expand Down Expand Up @@ -185,7 +186,10 @@ const previousConfigurationMetadata: ComponentConfigOutput[] = [
createConfig('@o3r/lib6', 'MyConfig6', ['prop6']),
createConfig('@o3r/lib7', 'MyConfig7', ['prop7']),
createConfig('@o3r/lib8', 'MyConfig8', ['prop8']),
createConfig('@o3r/lib9', 'MyConfig9', ['prop9'])
createConfig('@o3r/lib9', 'MyConfig9', ['prop9']),
// This case should not happen anymore as we filter config without properties
// Adding this case to ensure the support of older metadata
createConfig('@o3r/lib10', 'MyConfig10', [])
];

const newConfigurationMetadata: ComponentConfigOutput[] = [
Expand All @@ -198,7 +202,8 @@ const newConfigurationMetadata: ComponentConfigOutput[] = [
createConfig('@new/lib6', 'NewConfig6', ['newProp6Name']),
createConfig('@o3r/lib7', 'NewConfig7', ['prop7']),
createConfig('@new/lib8', 'MyConfig8', ['prop8']),
createConfig('@new/lib9', 'MyConfig9', ['prop9'])
createConfig('@new/lib9', 'MyConfig9', ['prop9']),
createConfig('@o3r/lib10', 'MyConfig10', ['prop10'])
];

async function writeFileAsJSON(path: string, content: object) {
Expand Down Expand Up @@ -298,6 +303,12 @@ const initTest = async (
await writeFileAsJSON(migrationDataPath, migrationData);
};

const getMessagesForId = (id: string) => ({
notDocumented: `Property ${id} has been modified but is not documented in the migration document`,
documentedButNotPresent: `Property ${id} has been modified but the new property is not present in the new metadata`,
breakingChangesNotAllowed: `Property ${id} is not present in the new metadata and breaking changes are not allowed`
});

describe('check metadata migration', () => {
test('should not throw', async () => {
await initTest(
Expand Down Expand Up @@ -344,11 +355,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).not.toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).not.toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand Down Expand Up @@ -379,11 +398,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).not.toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).not.toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand All @@ -408,11 +435,19 @@ describe('check metadata migration', () => {
} catch (e: any) {
/* eslint-disable jest/no-conditional-expect -- always called as there is a throw in the try block */
expect(e.message).not.toBe('should have thrown before');
previousConfigurationMetadata.slice(1).forEach(({ library, name, properties }) => {
const id = `${library}#${name} ${properties[0].name}`;
expect(e.message).not.toContain(`Property ${id} has been modified but is not documented in the migration document`);
expect(e.message).not.toContain(`Property ${id} has been modified but the new property is not present in the new metadata`);
expect(e.message).toContain(`Property ${id} is not present in the new metadata and breaking changes are not allowed`);
previousConfigurationMetadata.slice(1, -1).forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).toContain(breakingChangesNotAllowed);
});
[previousConfigurationMetadata[0], previousConfigurationMetadata.at(-1)].forEach((item) => {
const id = configMetadataComparator.getIdentifier(item);
const { notDocumented, documentedButNotPresent, breakingChangesNotAllowed } = getMessagesForId(id);
expect(e.message).not.toContain(notDocumented);
expect(e.message).not.toContain(documentedButNotPresent);
expect(e.message).not.toContain(breakingChangesNotAllowed);
});
/* eslint-enable jest/no-conditional-expect */
}
Expand Down
3 changes: 2 additions & 1 deletion packages/@o3r/configuration/schemas/configuration.metadata.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@
}
}
]
}
},
"minItems": 1
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/@o3r/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@
"@o3r/store-sync": "workspace:^",
"@stylistic/eslint-plugin": "~2.7.0",
"@types/jest": "~29.5.2",
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/parser": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"@typescript-eslint/parser": "~8.18.0",
"angular-eslint": "~18.4.0",
"cpy-cli": "^5.0.0",
"eslint": "~9.14.0",
Expand All @@ -176,7 +176,7 @@
"jest-preset-angular": "~14.2.0",
"jsonc-eslint-parser": "~2.4.0",
"nx": "~19.8.0",
"typescript-eslint": "~8.17.0",
"typescript-eslint": "~8.18.0",
"zone.js": "~0.14.2"
},
"engines": {
Expand Down
6 changes: 3 additions & 3 deletions packages/@o3r/eslint-config-otter/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@
"typescript": "^5.5.4"
},
"generatorDependencies": {
"@typescript-eslint/eslint-plugin": "~8.17.0",
"@typescript-eslint/parser": "~8.17.0",
"@typescript-eslint/utils": "~8.17.0",
"@typescript-eslint/eslint-plugin": "~8.18.0",
"@typescript-eslint/parser": "~8.18.0",
"@typescript-eslint/utils": "~8.18.0",
"eslint": "~9.14.0",
"eslint-plugin-jsdoc": "~50.2.0",
"eslint-plugin-unicorn": "^56.0.0"
Expand Down
Loading