feat(compare-config): add optional includeInherited property and apply it to comparison logic

[gaumrab]

Summary

Fixed inheritance chain comparison bug in Concerto analysis where inherited properties were incorrectly reported as "removed" when comparing model versions. Added includeInherited option to CompareConfig to properly handle inherited properties during model comparison.

Changes

• Added includeInherited?: boolean option to CompareConfig type in compare-config.ts
• Updated compare.ts to use getProperties() instead of getOwnProperties() when includeInherited is true
• Modified property comparison logic to consider inherited properties from parent classes
• Maintained backward compatibility by making includeInherited optional with default false

Flags

• This is a breaking change for users who rely on the current behavior of ignoring inherited properties
• Users must explicitly set includeInherited: true to get the new behavior
• Default behavior remains unchanged to maintain backward compatibility

Screenshots or Video

Related Issues

• Issue Concerto Analyser ignores the inheritance chain when comparing properties between two model versions #1067: Concerto Analyser ignores the inheritance chain when comparing properties between two model versions

Author Checklist

• Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
• Vital features and changes captured in unit and/or integration tests
• Commits messages follow AP format
• Extend the documentation, if necessary
• Merging to main from fork:branchname

[ekarademir]

@gaumrab Could you update the PR title to follow commit conventions please? https://www.conventionalcommits.org/en/v1.0.0/

[gaumrab]

sure @ekarademir

Is this looks good: feat(compare-config): add optional includeInherited property and apply it to comparison logic

according to the commit conventions.

let me know if still any change required.

[ekarademir]

@gaumrab could you also add test cases for the fix?

[ekarademir]

Why would you need this setting? Is there anything breaking do you think?

[gaumrab]

@ekarademir
I added the includeInherited property to accommodate two different user needs:

1. Users who need to compare only properties defined in the child class
2. Users who need to compare properties from both child and inherited classes

By making it optional, we satisfy both groups without introducing breaking changes. This ensures backward compatibility while providing the flexibility to handle inheritance chains when needed.

If You Want me to remove this Setting and directly want to use getProperties(), I wil Update.

But We can't Depends here directly on a getProperties().
If its return something un-relatable at that time User have Choice to Switch other Property getOwnProperties by using Setting includeInherited.

[gaumrab]

@erikbfeeley

To Test the new property exists:
Create CompareConfig with includeInherited: true
Verify it's accepted
Test the logic change:
Compare with includeInherited: false → should get MAJOR result
Compare with includeInherited: true → should get PATCH result
Test backward compatibility:
Default behavior (no includeInherited) → should work as before

You Want me to add unit test case for that.

Currently I didn't Pushed it but i am on my local using it

#!/usr/bin/env node
/*

• Licensed under the Apache License, Version 2.0 (the "License");
• you may not use this file except in compliance with the License.
• You may obtain a copy of the License at
• http://www.apache.org/licenses/LICENSE-2.0
• Unless required by applicable law or agreed to in writing, software
• distributed under the License is distributed on an "AS IS" BASIS,
• WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
• See the License for the specific language governing permissions and
• limitations under the License.
  */

'use strict';

const globPattern = process.argv[2];

const util = require('util');
const fs = require('fs');
const path = require('path');
const copyFilePromise = util.promisify(fs.copyFile);
const glob = require('glob');

function copyFiles(files, destDir) {
if (!fs.existsSync('coverage')){
fs.mkdirSync('coverage');
}
return Promise.all(files.map(f => {
return copyFilePromise(f.source, path.join(destDir, f.destination));
}));
}

const lcovs = glob.sync(globPattern).map((dir) => {
const packageName = dir.split(path.sep).join('/').split('/').pop();
return {
source: path.join(dir.split(path.sep).join('/'), 'coverage/coverage-final.json'),
destination: ${packageName}.json
};
});

// usage
copyFiles(lcovs, 'coverage').then(() => {
console.log("done");
}).catch(err => {
console.log(err);
});

[ekarademir]

@gaumrab it's best to push the change and have discussion about it. Once you push it it'll also run on our ci pipeline.

[gaumrab]

got it @ekarademir.

After it's done i will update you here.

[gaumrab]

Unit Tests Added for Inheritance Bug Fix

Hi @ekarademir ,

I've pushed unit tests for the inheritance comparison bug fix. The test file packages/concerto-analysis/test/unit/inheritance-bug-fix.test.ts verifies that the includeInherited option works correctly.

Status: CI pipeline is awaiting approval from a maintainer in #1069

Test Coverage:

• ✅ Bug reproduction (MAJOR result with default behavior)
• ✅ Bug fix verification (PATCH result with includeInherited: true)
• ✅ Behavior comparison between old/new approaches
• ✅ Property inheritance analysis

To test locally:

git clone https://github.com/gaumrab/concerto.git
cd concerto && git checkout fix/1067/inheritance-comparison-bug
npm install && cd packages/concerto-analysis
npm test -- test/unit/inheritance-bug-fix.test.ts

Could someone please approve the workflow? The inheritance bug fix is ready for review.

Thanks!

[gaumrab]

@ekarademir any update??

[github-actions]

This PR is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 10 days.