Skip to content

Conversation

@coryan
Copy link
Contributor

@coryan coryan commented Sep 28, 2023

This change is Reviewable

@coryan coryan temporarily deployed to internal September 28, 2023 14:55 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (b312cb4) 93.60% compared to head (de9bb5e) 93.60%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12752      +/-   ##
==========================================
- Coverage   93.60%   93.60%   -0.01%     
==========================================
  Files        2075     2075              
  Lines      182377   182377              
==========================================
- Hits       170715   170710       -5     
- Misses      11662    11667       +5     

see 3 files with indirect coverage changes

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

@coryan coryan marked this pull request as ready for review September 28, 2023 15:35
@coryan coryan requested a review from a team as a code owner September 28, 2023 15:35
GOOGLE_CLOUD_CPP_SPANNER_SLOW_INTEGRATION_TESTS="instance"
mapfile -t integration_args < <(integration::bazel_args)
mapfile -t integration_args < <(integration::bazel_coverage_args)
integration::bazel_with_emulators coverage "${args[@]}" "${integration_args[@]}"
Copy link
Member

@dbolduc dbolduc Sep 28, 2023

Choose a reason for hiding this comment

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

Consider leaving integration.sh alone and doing something like:

# With code coverage the `--flaky_test_attempts`
# works in unexpected ways. A flake produces an empty `coverage.dat` file, which
# breaks the build when trying to consolidate all the `coverage.dat` files.
#
# This combination of a "successful" test (because the flake is retried) and a
# failure later in the consolidation of coverage results easily leads the
# developer astray.
coverage_args=("${args[@]" "${integration_args[@]/--flaky_test_attempts=3/}")
integration::bazel_with_emulators coverage "${coverage_args[@]}" 

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't like the explicit 3, try ...

# blah blah blah
let i=0
for arg in "${integration_args[@]}"
do
  case "${arg}" in
  --flaky_test_attempts=*)
    unset integration_args[$i]
    ;;
  esac
  let ++i
done

This also has the advantage of removing the array element, rather than replacing it with an empty string.

@coryan coryan temporarily deployed to internal September 28, 2023 16:54 — with GitHub Actions Inactive
@coryan coryan temporarily deployed to internal September 28, 2023 17:06 — with GitHub Actions Inactive
@coryan coryan enabled auto-merge (squash) September 28, 2023 17:17
@coryan coryan merged commit da7cecd into googleapis:main Sep 28, 2023
@coryan coryan deleted the ci-no-retry-in-coverage-build branch September 28, 2023 18:06
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