Skip to content

Conversation

khieta
Copy link
Contributor

@khieta khieta commented Mar 13, 2024

Description of changes

Issue #, if available

Checklist for requesting a review

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

  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

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):

  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

@aaronjeline aaronjeline left a comment

Choose a reason for hiding this comment

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

tiny nits

@@ -0,0 +1,25 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to have main.rs instead of lib.rs? this is a library crate, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No special reason, I was just copying the structure of the CLI tests. The files just contain #[test]s. Is lib or main more appropriate? Or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

lib.rs is used for crates that provide libraries, main.rs is used for crates compiling an executable. (main.rs usually contains a main() -- in fact, I'm surprised Cargo isn't complaining that this one doesn't.) See https://doc.rust-lang.org/cargo/guide/project-layout.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on that link, I think that using a main.rs file in the tests subfolder is the correct structure.

If a binary, example, bench, or integration test consists of multiple source files, place a main.rs file along with the extra modules within a subdirectory of the src/bin, examples, benches, or tests directory. The name of the executable will be the directory name.

Also learned: the reason that the cedar-policy integration tests weren't running after #707 was the nested directory structure I introduced under tests/.

Signed-off-by: Kesha Hietala <[email protected]>
@khieta
Copy link
Contributor Author

khieta commented Mar 14, 2024

Fix for the CedarDRT build failure in cedar-spec#253.

@khieta khieta dismissed aaronjeline’s stale review March 14, 2024 20:08

comments addressed

@khieta khieta merged commit 1ef5225 into main Mar 14, 2024
@khieta khieta deleted the khieta/more-int-tests branch March 14, 2024 20:09
john-h-kastner-aws pushed a commit that referenced this pull request May 7, 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.

4 participants