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

fix missing override for InventoryReportRenderer #304

Merged

Conversation

balrok
Copy link
Contributor

@balrok balrok commented May 13, 2024

After introducing the delayed loading of the overrides file with PR #286, the overrides for the first dependency in the InventoryReportRenderer were not applied.

It is not entirely clear to me, why this happens. I just assume it has to do with groovy-magic related to automatically calling the getters of properties.

To resolve this, the base-property was renamed, so it doesn't conflict with the getter.

For writing the tests, I updated to spock 2.4 because it has the Snapshot-Testing Feature. I implemented it like this: when executing it locally, the tests will always become green but may update the snapshot-files. When running in the CI the tests will become red if the snapshot changed. I think it is an OK solution because the snapshot-files are checked into git anyways and even if the tests locally won't fail, a developer will see that things changed.

Also I updated the github actions because their version were mentioned to be deprecated.

@balrok balrok marked this pull request as draft May 13, 2024 06:55
@balrok balrok force-pushed the bugfix/fix-overrides-for-inventoryreportrenderer branch 3 times, most recently from dad1be4 to 8a01186 Compare May 16, 2024 21:09
After introducing the delayed loading of the overrides file with
PR jk1#286, the overrides for the first dependency in the
InventoryReportRenderer were not applied.

It is not entirely clear to me, why this happens. I just assume it has
to do with groovy-magic related to automatically calling the getters of
properties.

To resolve this, the base-property was renamed, so it doesn't conflict
with the getter.
@balrok balrok force-pushed the bugfix/fix-overrides-for-inventoryreportrenderer branch 2 times, most recently from bd729cd to 0da07a0 Compare May 16, 2024 21:55
@balrok balrok force-pushed the bugfix/fix-overrides-for-inventoryreportrenderer branch from 0da07a0 to 0608d94 Compare May 16, 2024 21:59
@balrok balrok marked this pull request as ready for review May 17, 2024 08:08
@jk1 jk1 merged commit bd5c7c2 into jk1:master May 26, 2024
1 check failed
@jk1
Copy link
Owner

jk1 commented May 26, 2024

Thank you for the contribution. The snapshot testing is indeed a concept the plugin can benefit from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants