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.

Sign up for GitHub

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

[CI only] Streamlined/fixed sanitize/coverage matrices #790

Merged
merged 1 commit into from
Aug 13, 2024
Merged

[CI only] Streamlined/fixed sanitize/coverage matrices #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions .github/workflows/build-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,18 @@ on:
dev_mode:
type: string
default: "OFF"
lib_msg_delivery:
pool_dispatch:
type: string
default: "OFF"
lib_write_deadline:
default: "NO-pool"
write_deadline:
type: string
default: "OFF"
lib_force_host_verification:
default: "NO-write_deadline"
tls:
type: string
default: "ON"
default: "TLS"
verify_host:
type: string
default: "verify_host"
repeat:
type: string
default: "1"
Expand All @@ -35,9 +38,6 @@ on:
streaming:
type: string
default: "ON"
tls:
type: string
default: "ON"
type:
type: string
description: "Debug or Release."
Expand Down Expand Up @@ -88,11 +88,20 @@ jobs:
flags: -DNATS_BUILD_ARCH=${{ inputs.arch }}
-DCMAKE_BUILD_TYPE=${{ inputs.type }}
-DNATS_BUILD_STREAMING=${{ inputs.streaming }}
-DNATS_BUILD_WITH_TLS=${{ inputs.tls }}
-DNATS_PROTOBUF_DIR=${{ github.workspace}}/deps/pbuf
-DNATS_BUILD_USE_SODIUM=ON
-DNATS_SODIUM_DIR=${{ github.workspace}}/deps/sodium
run: |
if [[ "${{ inputs.tls }}" == "TLS" ]]; then
flags="$flags -DNATS_BUILD_WITH_TLS=ON"
if [[ "${{ inputs.verify_host }}" == "verify_host" ]]; then
flags="$flags -DNATS_BUILD_TLS_FORCE_HOST_VERIFY=ON"
else
flags="$flags -DNATS_BUILD_TLS_FORCE_HOST_VERIFY=OFF"
fi
else
flags="$flags -DNATS_BUILD_WITH_TLS=OFF"
fi
if [[ -n "${{ inputs.sanitize }}" ]]; then
flags="$flags -DNATS_SANITIZE=ON -DCMAKE_C_FLAGS=-fsanitize=${{ inputs.sanitize }}"
fi
Expand All @@ -113,15 +122,12 @@ jobs:
if [[ -n "${{ inputs.sanitize }}" ]]; then
echo "NATS_TEST_VALGRIND=yes" >> $GITHUB_ENV
fi
if [[ "${{ inputs.lib_msg_delivery }}" == "ON" ]]; then
if [[ "${{ inputs.pool_dispatch }}" == "pool" ]]; then
echo "NATS_DEFAULT_TO_LIB_MSG_DELIVERY=yes" >> $GITHUB_ENV
fi
if [[ "${{ inputs.lib_write_deadline }}" == "ON" ]]; then
if [[ "${{ inputs.write_deadline }}" == "write_deadline" ]]; then
echo "NATS_DEFAULT_LIB_WRITE_DEADLINE=2000" >> $GITHUB_ENV
fi
if [[ "${{ inputs.lib_force_host_verification }}" == "ON" ]]; then
echo "NATS_FORCE_HOST_VERIFICATION=yes" >> $GITHUB_ENV
fi
echo "CC=${{ inputs.compiler }}" >> $GITHUB_ENV

# install build dependencies
Expand Down
73 changes: 40 additions & 33 deletions .github/workflows/on-pr-debug.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,6 @@ jobs:
server_version: main
type: Debug

coverage:
name: "Coverage"
uses: ./.github/workflows/build-test.yml
with:
coverage: ON
server_version: main
type: Debug
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

coverage-pooled:
name: "Coverage (pooled delivery)"
uses: ./.github/workflows/build-test.yml
with:
coverage: ON
server_version: main
type: Debug
lib_msg_delivery: ON
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

dev-mode:
name: "DEV_MODE"
uses: ./.github/workflows/build-test.yml
Expand All @@ -56,33 +35,61 @@ jobs:
matrix:
compiler: [gcc, clang]
sanitize: [address, thread]
pooled_delivery: [ON, OFF]
pooled_dispatch: [pool, NO-pool]
uses: ./.github/workflows/build-test.yml
with:
server_version: main
type: Debug
type: RelWithDebInfo
compiler: ${{ matrix.compiler }}
sanitize: ${{ matrix.sanitize }}
lib_msg_delivery: ${{ matrix.pooled_delivery }}
pool_dispatch: ${{ matrix.pooled_dispatch }}

sanitize-write-deadline:
name: "Sanitize address (write_deadline)"
coverage-TLS:
name: "Coverage: TLS"
strategy:
fail-fast: false
matrix:
pooled_dispatch: [pool, NO-pool]
write_deadline: [write_deadline, NO-write_deadline]
uses: ./.github/workflows/build-test.yml
with:
coverage: ON
type: RelWithDebInfo
sanitize: address
server_version: main
lib_write_deadline: ON
compiler: gcc
tls: TLS
verify_host: verify_host
pool_dispatch: ${{ matrix.pooled_dispatch }}
write_deadline: ${{ matrix.write_deadline }}
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

sanitize-no-host-verify:
name: "Sanitize address (NO force_host_verification)"
coverage-NO-verify_host:
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a single coverage run (basically with the defaults of the library that has TLS/enfore_hostname_verification/etc..)?
Or is it that we would get a much lower score (for all code that is in the "#else" of those "#ifdef") without these new runs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. What prompted this was the refactoring of deliveries - the thread pool refactor code generated a lot of missing coverage comments since it was not ran with coverage.

My thinking: the coverage overhead is relatively small (relative to sanitize) so it's ok to run a bigger matrix. For the same reason, the coverage runs are probably sufficient to assert functional correctness (no need to dial down the parameters, for example). We still run the no-coverage tests for the default build config.

FWIW, I failed to package them into a single compact matrix statement, but the current job set is probably more intention revealing anyway then the matrix would have been.

name: "Coverage: NO-verify_host"
uses: ./.github/workflows/build-test.yml
with:
coverage: ON
type: RelWithDebInfo
sanitize: address
server_version: main
lib_force_host_verification: OFF

compiler: gcc
tls: TLS
verify_host: NO-verify_host
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

coverage-NO-TLS:
name: "Coverage NO-TLS"
uses: ./.github/workflows/build-test.yml
with:
coverage: ON
type: RelWithDebInfo
server_version: main
compiler: gcc
tls: NO-TLS
verify_host: NO-verify_host
secrets:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

bench:
name: "Benchmark"
uses: ./.github/workflows/build-test.yml
Expand Down