power-grid-model icon indicating copy to clipboard operation
power-grid-model copied to clipboard
verified publisher icon PowerGridModel

Feature/Re route validation tests

Open figueroa1395 opened this issue 1 year ago • 10 comments

Re-route the validation tests through the C API:

  • [x] Move the call for the tests to the C++ wrapper of the C API.
  • [x] Remove the PGM core dependencies.

~~Re-route the benchmarks through the C API:~~ ~~- [ ] Move the call for the benchmark to the C++ wrapper of the C API.~~ ~~- [ ] Remove the PGM core dependencies.~~

Note: The re-routing of the benchmark won't be addressed here anymore, since it requieres more effort than initially anticipated. For the moment, it is taken out of the CI build and addressed on the nightly build #736

Closes #486

figueroa1395 avatar Sep 11 '24 15:09 figueroa1395

@figueroa1395 should the name of this PR be changed to only re-route validation tests?

TonyXiang8787 avatar Sep 26 '24 09:09 TonyXiang8787

@figueroa1395 should the name of this PR be changed to only re-route validation tests?

PR edited accordingly.

figueroa1395 avatar Sep 26 '24 09:09 figueroa1395

Since I have started working on this PR again, this is very outdated at the moment. Many things have and will keep changing.

figueroa1395 avatar Sep 26 '24 11:09 figueroa1395

At this stage all the single validation tests have been migrated to the Cpp API and pass. Next I will fix the batch validation.

figueroa1395 avatar Sep 30 '24 14:09 figueroa1395

@figueroa1395 you need to resolve merge conflict, otherwise the CI will not start.

TonyXiang8787 avatar Oct 02 '24 06:10 TonyXiang8787

@TonyXiang8787 Thanks, I did not know. I am doing a bit of refactoring and once I finish that, I will solve the conflict.

figueroa1395 avatar Oct 02 '24 06:10 figueroa1395

@figueroa1395 I would suggest you to first do a down-merge to resolve the merge conflict, then refactoring. After refactoring the conflict will be more difficult to resolve.

TonyXiang8787 avatar Oct 02 '24 07:10 TonyXiang8787

@TonyXiang8787 Merge conflict resolved and refactoring done. Just one thing remains: Previously, the batch validation tests were tested not only for a whole update (which is the current state), but also on a per-scenario basis, is this still needed or is it sufficient to validate the results after the whole update has been done?

figueroa1395 avatar Oct 02 '24 10:10 figueroa1395

@TonyXiang8787 Merge conflict resolved and refactoring done. Just one thing remains: Previously, the batch validation tests were tested not only for a whole update (which is the current state), but also on a per-scenario basis, is this still needed or is it sufficient to validate the results after the whole update has been done?

No, I think that's not needed.

TonyXiang8787 avatar Oct 02 '24 10:10 TonyXiang8787

The only things remaining at this point are doing a full manual clang tidy run and figuring out what is going on with the MacOS build.

figueroa1395 avatar Oct 04 '24 15:10 figueroa1395

The only things remaining at this point are doing a full manual clang tidy run and figuring out what is going on with the MacOS build.

@figueroa1395 @mgovers After debugging in my Mac I have found the bug. The std::aligned_alloc has two requirements for unix-like system which we did not enforce:

  • Alignment should be multiple of sizeof(void*)
  • Size should be multiple of alignment

I have now pushed the fix.

TonyXiang8787 avatar Oct 05 '24 20:10 TonyXiang8787

@TonyXiang8787 Thanks! Great find and fix.

figueroa1395 avatar Oct 07 '24 06:10 figueroa1395

Quality Gate Passed Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Oct 07 '24 10:10 sonarqubecloud[bot]