Skip to content

Add feature to disable building select C++ tests #57

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

Closed
rtmadduri wants to merge 2 commits into from
Closed

Add feature to disable building select C++ tests #57

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8847bc9
add test exceptions
rtmadduri Nov 17, 2025
951aa10
address copilot comment
rtmadduri Nov 17, 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
11 changes: 11 additions & 0 deletions libflashinfer/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# Set global paths and initialize test list
set(ALL_TEST_TARGETS "")

# List of tests to skip These tests are currently disabled due to known issues
Comment thread
rtmadduri marked this conversation as resolved.
Outdated
# or pending fixes.
set(SKIP_TESTS test_batch_prefill test_layout_transform
test_mfma_fp32_16x16x16fp16 test_cascade)
Comment thread
rtmadduri marked this conversation as resolved.
Outdated

# Include centralized config utilities
include(ConfigureTargets)

Expand All @@ -13,6 +18,12 @@ if(FLASHINFER_ENABLE_HIP)

foreach(test_source IN LISTS HIP_TEST_SOURCES)
get_filename_component(test_name ${test_source} NAME_WE)

if(test_name IN_LIST SKIP_TESTS)

@diptorupd diptorupd Nov 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I do not think this is the right approach. I will still like to have build targets for the broken tests. Having a build target will help during development when a broken test is getting fixed or otherwise needs reevaluation.

I suggest the target should not be added to the build_tests convenience target that builds all tests in one shot. That way CI or otherwise can still build all "good" tests in one go, but a developer has the option to easily try out a broken test using cmake.

@rtmadduri rtmadduri Nov 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Are you suggesting something like

foreach(test_target IN LISTS ALL_TEST_TARGETS)
  if(TARGET ${test_target})
    if(test_name IN_LIST SKIP_TESTS)
        message(STATUS "Skipping HIP test: ${test_name}")
        continue()
    endif()
    gtest_discover_tests(${test_target})
  endif()
endforeach()

@rtmadduri rtmadduri Nov 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Wouldnt this only work for tests that build successfully but fail during runtime? Right now test_batch_prefill does not even build because changes to BatchPrefillWithPagedKVCache did not go in yet.

message(STATUS "Skipping HIP test: ${test_name}")
continue()
endif()

set(target_name "${test_name}_hip")
configure_flashinfer_target(
TARGET_NAME ${target_name}
Expand Down