Skip to content

IDX-65: Fix the execution of the Cedar integration tests #1

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

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

patjakdev
Copy link

@patjakdev patjakdev commented Jul 3, 2024

There are myriad different issues addressed in this PR, but the result is that the corpus tests now run successfully and pass:

  1. We now use shell: bash in the Github action. This sets the -o pipefail behavior that you might expect to be a default (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference). Without this setting, go test -count=1 -v -tags corpus ./integration_tests/... | tail will always return a success error code.
  2. As written, the Github action would create an issue any time the integration tests failed, even if they were run manually. We really only want to create issues if the automated nightly tests start failing.
  3. The number of corpus tests has dropped below our threshold for assuming we've somehow screwed up the extraction of the tests from the published tarfile. I updated the threshold to 100, an order of magnitude below the current count of tests. The commit that reduced the test count appears to have been this one.
  4. Fixes a casing error in the comparison with the decision string from the test.

This sets the `-o pipefail` behavior that you might expect to be a default (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference). Without this setting, `go test -count=1 -v -tags corpus ./integration_tests/... | tail` will always return a success error code.
@patjakdev patjakdev force-pushed the idx-65/fix-corpus-alerts branch from 5c11f78 to ac04178 Compare July 3, 2024 00:33
patjakdev added 2 commits July 2, 2024 17:39
…d run.

Right now, the only other way to run the tests is to invoke them manually. If I'm running them against some experiment in my branch, I don't want to alert anybody about the failure.
…led in some fatal way.

Commit [b6e8040](cedar-policy/cedar-integration-tests@b6e8040) reduced the size of the tgz file by 25%, which was the likely culprit in reducing the size of the test suite.
@patjakdev patjakdev marked this pull request as ready for review July 3, 2024 00:48
@patjakdev patjakdev requested review from jmccarthy and philhassey July 3, 2024 00:49
@patjakdev patjakdev changed the title IDX-65: Set default shell to bash IDX-65: Fix the execution of the Cedar integration tests Jul 3, 2024
Copy link

@philhassey philhassey left a comment

Choose a reason for hiding this comment

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

For lack of knowing the exact syntax of the YML, it looks good. I recommend perusing this PR from the AWS Cedar team, as it includes some other minor tweak relating to this.

cedar-policy#16

@patjakdev
Copy link
Author

For lack of knowing the exact syntax of the YML, it looks good. I recommend perusing this PR from the AWS Cedar team, as it includes some other minor tweak relating to this.

cedar-policy#16

@philhassey Thanks. I've incorporated the other changes they made into my PR and confirmed that the tests continue to pass.

@patjakdev patjakdev merged commit 00ee481 into main Jul 3, 2024
5 checks passed
@patjakdev patjakdev deleted the idx-65/fix-corpus-alerts branch July 3, 2024 22:39
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.

2 participants