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

feat(test): refactor all PolicyDefinitionStore implementors tests by using a base class #1959

Conversation

wolf4ood
Copy link
Contributor

What this PR changes/adds

Add a common base test class for PolicyDefinitionStore which all implementors should extends

Why it does that

To have a common and consistent base specification test suite for all implementors of PolicyDefinitionStore

Further notes

  • The impl PolicyDefinitionStore#deleteById of CosmosPolicyDefinitionStore has been changed by returning null instead of throwing an exception if the PolicyDefinition is not found
  • The following base tests were ignored in InMemory and Comos impl since they do not support yet querying collections fields.
    • find_queryByDuties
    • find_queryByPermissions
    • find_queryByProhibitions
  • The following base tests were ignored in the Posgres impl since it does not support yet sorting.
    • findAll_whenSort
    • findAll_allFilters
  • In PolicyDefinitionMapping added id mapping for compatibility with InMemory implementation.

List other areas of code that have changed but are not necessarily linked to the main feature. This could be method signature changes, package declarations, bugs that were encountered and were fixed inline, etc.

Linked Issue(s)

Closes #1941

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@wolf4ood wolf4ood added the enhancement New feature or request label Sep 13, 2022
@wolf4ood wolf4ood temporarily deployed to Azure-dev September 13, 2022 14:47 Inactive
@codecov-commenter
Copy link

Codecov Report

Merging #1959 (72b40b6) into main (67f3b8a) will increase coverage by 0.00%.
The diff coverage is 75.00%.

@@           Coverage Diff           @@
##             main    #1959   +/-   ##
=======================================
  Coverage   62.79%   62.79%           
=======================================
  Files         782      782           
  Lines       16612    16618    +6     
  Branches     1080     1080           
=======================================
+ Hits        10432    10436    +4     
- Misses       5729     5731    +2     
  Partials      451      451           
Impacted Files Coverage Δ
...osmos/policy/store/CosmosPolicyStoreExtension.java 0.00% <0.00%> (ø)
...smos/policy/store/CosmosPolicyDefinitionStore.java 91.30% <77.77%> (-6.32%) ⬇️
...store/schema/postgres/PolicyDefinitionMapping.java 100.00% <100.00%> (ø)
.../policy/store/schema/SqlPolicyStoreStatements.java 92.30% <0.00%> (+7.69%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once the conflicts are resolved I'll merge it

@wolf4ood wolf4ood force-pushed the feature/1941_consolidate_policydefinitionstore_tests branch from 72b40b6 to 2f2ff2b Compare September 14, 2022 07:53
@wolf4ood wolf4ood temporarily deployed to Azure-dev September 14, 2022 07:53 Inactive
@jimmarino jimmarino merged commit 027c03a into eclipse-edc:main Sep 14, 2022
@wolf4ood wolf4ood deleted the feature/1941_consolidate_policydefinitionstore_tests branch October 12, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subtask: Consolidate tests for PolicyDefinitionStore
4 participants