fix: Revert "fix: remove t-algorithms-test" - #2557
Draft
wdconinc wants to merge 9 commits into
Draft
Conversation
- Exclude t_algorithms_test from the build job's testsuite for ASAN builds (using --exclude-regex), so the build job doesn't fail and all downstream jobs can continue running. The test still runs in non-ASAN builds (TSAN, Debug/coverage) so llvm-cov continues to work. - Upload the algorithms_test binary as a separate artifact for ASAN builds. - Add a new t-algorithms-test job (continue-on-error: true, no dependents) that downloads the install+binary artifacts and runs t_algorithms_test to track the failure status. Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
wdconinc
force-pushed
the
copilot/exclude-t-algorithms-test-revert
branch
from
March 11, 2026 15:23
c5b9353 to
7776c88
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Re-enables a dedicated GitHub Actions job to run the t_algorithms_test diagnostics separately from the main build/test job in the eic-shell CI workflow.
Changes:
- Adds a new
t-algorithms-testjob that downloads ASAN build artifacts and runs thealgorithms_testbinary. - Keeps the main
buildjob excludingt_algorithms_testunder ASAN while providing a separate place to observe failures (non-blocking viacontinue-on-error).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
The `t-algorithms-test` workflow job invoked the `algorithms_test` binary directly, bypassing the test definition set in CMake (including `LLVM_PROFILE_FILE` and any other future test properties). ## Change Instead of downloading just the test binary and running it directly, the build job now produces a slim build archive (containing the `CTestTestfile.cmake` hierarchy and the test binary) for the `clang++ Release ASAN` configuration. The `t-algorithms-test` job downloads this archive and runs the test via `ctest`, exactly as defined in `src/tests/algorithms_test/CMakeLists.txt`: ```cmake add_test(NAME t_algorithms_test COMMAND env LLVM_PROFILE_FILE=algorithms_test.profraw $<TARGET_FILE:algorithms_test>) ``` The workflow step now runs: ```yaml ctest --test-dir build -V -R "^t_algorithms_test$" ``` This ensures the job automatically stays in sync with any future changes to the CMake test definition (new environment variables, working directory, test properties, etc.) without requiring manual updates to the workflow. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+221
to
+229
| - name: Compress build directory (for t_algorithms_test) | ||
| if: ${{ matrix.CXX == 'clang++' && matrix.CMAKE_BUILD_TYPE == 'Release' && matrix.USE_ASAN == 'ON' && matrix.USE_TSAN == 'OFF' }} | ||
| run: tar -caf build-t-algorithms-test.tar.zst $(find build -name "CTestTestfile.cmake") build/src/tests/algorithms_test/algorithms_test | ||
| - name: Upload build directory (for t_algorithms_test) | ||
| if: ${{ matrix.CXX == 'clang++' && matrix.CMAKE_BUILD_TYPE == 'Release' && matrix.USE_ASAN == 'ON' && matrix.USE_TSAN == 'OFF' }} | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: build-t-algorithms-test-${{ matrix.CXX }}-eic-shell-${{ matrix.CMAKE_BUILD_TYPE }}-${{ env.platform }}-${{ matrix.release }}-ASAN | ||
| path: build-t-algorithms-test.tar.zst |
veprbl
marked this pull request as draft
March 30, 2026 15:25
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Briefly, what does this PR introduce?
This reverts commit 492d5e3 ane re-enables the t-algorithms-test for diagnostics. The goal is that this branch contains the fix too.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No.
Does this PR change default behavior?
No.