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

update integration tests for #814 #819

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Update integration tests for the changes in #814. This was missed in #814 itself, perhaps we should check our CI?

Issue #, if available

#814

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A bug fix or other functionality change requiring a patch to cedar-policy.

I confirm that this PR (choose one, and delete the other options):

  • Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@khieta
Copy link
Contributor

khieta commented Apr 25, 2024

The CI is currently using https://github.com/cedar-policy/cedar-integration-tests (only Java is still using the old folder in this repo). I would have expected CI to fail though -- do integration tests run successfully locally? You can run them locally following the instruction in the README here https://github.com/cedar-policy/cedar/tree/main/cedar-testing

@cdisselkoen
Copy link
Contributor Author

Well, in #814 the Java CI failed, which was expected (the Java updates for #814 are in cedar-policy/cedar-java#129). Your comment explains why only the Java CI failed, and not the CI in this repo. We should at some point fix the Java to use https://github.com/cedar-policy/cedar-integration-tests like everything else, and then delete this folder, then?

@khieta
Copy link
Contributor

khieta commented Apr 25, 2024

Yes, the plan is to delete the folder here -- I should make an issue to track. The current blocker is that the new integration test repo uses the new schema format, which the Java doesn't support. I figured we'd add support for this in the Java as part of the updates for #757.

Edit: made issue to track: #820

@cdisselkoen
Copy link
Contributor Author

Java CI is failing on this PR due to cedar-policy/cedar-java#129 not being merged yet. This is expected. I will merge this, even though there is not really any specific test that this PR solved the issue it's trying to address. Worst case, we need another followup PR.

@cdisselkoen cdisselkoen merged commit 575e192 into main Apr 25, 2024
14 of 16 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/camelCaseFFI branch April 25, 2024 18:21
cdisselkoen added a commit that referenced this pull request Apr 25, 2024
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.

5 participants