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

chore(libsinsp/test): use GTEST_SKIP to track disabled tests #2158

Closed
wants to merge 3 commits into from
Closed

chore(libsinsp/test): use GTEST_SKIP to track disabled tests #2158

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89484c2
chore(libsinsp/test): use GTEST_SKIP to track disabled tests
therealbobo Nov 13, 2024
f598e25
fix(ci): always build on ubuntu 22.04
therealbobo Nov 28, 2024
e064c96
Revert "fix(ci): always build on ubuntu 22.04"
therealbobo Jan 8, 2025
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
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ jobs:
build-libs-linux:
name: build-libs-linux-${{ matrix.arch }} 😁 (${{ matrix.name }})
runs-on: ${{ (matrix.arch == 'arm64' && 'github-arm64-2c-8gb') || 'ubuntu-22.04' }}
container:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is another container below: https://github.com/falcosecurity/libs/pull/2158/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR38

Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, do we even need the container? I mean, we already run on ubuntu-22.04 on x86 and arm64.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the container directive.
Also, remember to add sudo before install deps steps

Copy link
Contributor

@FedeDP FedeDP Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why did you use container image ubuntu-22.04? can't we run directly on runner runs-on ?

Copy link
Contributor

Choose a reason for hiding this comment

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

/hold

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a grpc-related sanitizer issue otherwise :/ grpc/grpc#22783

It's ok with the container to workaround it!
/unhold

image: ubuntu:22.04
strategy:
fail-fast: false
matrix:
Expand All @@ -33,12 +35,10 @@ jobs:
cmake_opts: -DUSE_ASAN=On -DUSE_UBSAN=On -DUSE_BUNDLED_DEPS=False
- name: zig
cmake_opts: -DUSE_BUNDLED_DEPS=True
container:
image: debian:buster
steps:
- name: Install deps ⛓️
run: |
apt update && apt install -y --no-install-recommends curl ca-certificates build-essential git clang llvm pkg-config autoconf automake libtool libelf-dev wget libc-ares-dev libcurl4-openssl-dev libssl-dev libtbb-dev libjq-dev libjsoncpp-dev libgrpc++-dev protobuf-compiler-grpc libgtest-dev libprotobuf-dev linux-headers-${{ matrix.arch }}
apt update && apt install -y --no-install-recommends curl ca-certificates build-essential git clang llvm pkg-config autoconf automake libtool libelf-dev wget libc-ares-dev libcurl4-openssl-dev libssl-dev libtbb-dev libjq-dev libjsoncpp-dev libgrpc++-dev protobuf-compiler-grpc libgtest-dev libprotobuf-dev linux-headers-$(uname -r)

- name: Install a recent version of CMake ⛓️
run: |
Expand Down
17 changes: 8 additions & 9 deletions userspace/libsinsp/test/async_key_value_source.ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,16 +335,16 @@ TEST(async_key_value_source_test, look_key_delayed_async_callback) {

/**
* Ensure that "old" results are pruned
* This test usually fails like this when runned with sanitizers on arm64:
*
* pure virtual method called
* terminate called without an active exception
* Aborted (core dumped)
*
* Disabled until we can figure out how to fix it.
*/
#if !defined(__aarch64__)
TEST(async_key_value_source_test, prune_old_metadata) {
#if defined(__aarch64__)
GTEST_SKIP() << R"(This test usually fails like this when runned with sanitizers on arm64:
pure virtual method called
terminate called without an active exception
Aborted (core dumped)

Disabled until we can figure out how to fix it.)";
#endif
const uint64_t DELAY_MS = 0;
const uint64_t TTL_MS = 20;

Expand Down Expand Up @@ -380,7 +380,6 @@ TEST(async_key_value_source_test, prune_old_metadata) {
// fetch the first key should also return false.
ASSERT_FALSE(source.lookup(key1, response));
}
#endif

struct result {
uint64_t val = 0;
Expand Down
Loading