Skip to content

Conversation

@reyhankoyun
Copy link
Contributor

@reyhankoyun reyhankoyun commented Oct 20, 2025

Issue #, if available:

Description of changes:
Added caching integration tests:

  • test_cache_after_secret_update: Real AWS secret rotation + cache staleness
  • test_real_ttl_expiration_timing: Real time-based TTL with actual delays

Fixed GitHub Actions security vulnerability:

  • Changed pull_request_target to only trigger on labeled events
  • Eliminates race condition where unapproved code could execute with AWS credentials
  • Each commit now requires explicit human approval via safe-to-test label
  • Auto-removes label after use to prevent persistent approval

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- test_cache_hit_behavior: Verifies cache hits are faster than AWS calls
- test_refresh_now_bypasses_cache: Confirms refreshNow=true bypasses cache
- test_cache_after_secret_update: Tests stale cache behavior after secret updates
- test_real_ttl_expiration_timing: Validates TTL expiration and cache refresh
- test_ttl_zero_disables_caching: Ensures TTL=0 disables caching completely

These tests cover all critical caching behaviors that cannot be unit tested,
including timing-based assertions and AWS integration scenarios.
@reyhankoyun reyhankoyun requested a review from a team as a code owner October 20, 2025 20:15
@codecov
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.72%. Comparing base (cde666a) to head (019acf2).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #138   +/-   ##
=======================================
  Coverage   91.72%   91.72%           
=======================================
  Files          14       14           
  Lines        2418     2418           
  Branches     2418     2418           
=======================================
  Hits         2218     2218           
  Misses        150      150           
  Partials       50       50           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@simonmarty simonmarty added safe-to-test Maintainer approval to run integration tests for external contributor PRs. and removed safe-to-test Maintainer approval to run integration tests for external contributor PRs. labels Oct 23, 2025
- Updated caching.rs to only include true integration tests:
  - test_cache_after_secret_update: Real AWS secret rotation + cache staleness
  - test_real_ttl_expiration_timing: Real time-based TTL with actual delays
  - Removed performance-focused tests (moved to future performance suite)
  - Removed parameter behavior tests (moved to future unit tests)

- Fixed GitHub Actions security vulnerability:
  - Changed pull_request_target to only trigger on 'labeled' events
  - Eliminates race condition where unapproved code could execute with AWS credentials
  - Each commit now requires explicit human approval via safe-to-test label
  - Auto-removes label after use to prevent persistent approval

Integration tests now focus on real AWS interactions and timing behavior
that cannot be effectively mocked or measured in unit tests.
@reyhankoyun reyhankoyun added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 23, 2025
- name: Remove safe-to-test label after use
if: github.event_name == 'pull_request_target' && contains(github.event.pull_request.labels.*.name, 'safe-to-test')
run: |
gh api repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels/safe-to-test -X DELETE || true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to silence all errors that come out of the GitHub API

You can use something like

          gh_status=$(gh api "repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels/safe-to-test" -X DELETE | jq ".status" --raw-output)
          case $gh_status in
            200) echo "Label removed" ;;
            404) echo "Label not found — ignoring" ;;
            *) echo "unexpected HTTP $gh_status" && exit 1 ;;

to avoid that

pull-requests: write

steps:
- name: Remove safe-to-test label after use
Copy link
Contributor

Choose a reason for hiding this comment

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

One issue with doing this is that this will prevent re-driving integration workflows that fail due to causes outside of the code. Say there's an AWS outage while a workflow gets run, the label will get removed, then it will have to get re-added so that a new PR workflow can get triggered on the same commit, when in fact that relabeling should not be necessary from a safety standpoint, the commit has already been approved for integ tests by a person.

One way to get around this is to remove the safe-to-test label on the synchronize event like here

*Issue #, if available:*

*Description of changes:* Extract the fix in
aws@3f073a4


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Simon Marty <martysi@amazon.com>
@simonmarty simonmarty added safe-to-test Maintainer approval to run integration tests for external contributor PRs. and removed safe-to-test Maintainer approval to run integration tests for external contributor PRs. labels Oct 25, 2025
*Issue #, if available:*

*Description of changes:* Extract the fix in
aws@3f073a4

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
Signed-off-by: Simon Marty <martysi@amazon.com>
@simonmarty simonmarty added safe-to-test Maintainer approval to run integration tests for external contributor PRs. and removed safe-to-test Maintainer approval to run integration tests for external contributor PRs. labels Oct 25, 2025
@github-actions github-actions bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
*Issue #, if available:*

*Description of changes:* Extract the fix in
aws@3f073a4

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
simonmarty and others added 2 commits October 24, 2025 17:45
Signed-off-by: Simon Marty <martysi@amazon.com>
Signed-off-by: Simon Marty <simon.marty@protonmail.com>
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
@github-actions github-actions bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
@github-actions github-actions bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
Signed-off-by: Simon Marty <martysi@amazon.com>
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
@github-actions github-actions bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Oct 25, 2025
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