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.

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

[github] Enable assertions on test workflow #74849

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

sudonatalie
Copy link
Member

No description provided.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 8, 2023

@llvm/pr-subscribers-github-workflow

Author: Natalie Chouinard (sudonatalie)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/74849.diff

1 Files Affected:

  • (modified) .github/workflows/llvm-project-tests.yml (+1-1)
diff --git a/.github/workflows/llvm-project-tests.yml b/.github/workflows/llvm-project-tests.yml
index 6751bde4a11a9..02b1ab75e960e 100644
--- a/.github/workflows/llvm-project-tests.yml
+++ b/.github/workflows/llvm-project-tests.yml
@@ -96,7 +96,7 @@ jobs:
           # This should be a no-op for non-mac OSes
           PKG_CONFIG_PATH: /usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig//12
         with:
-          cmake_args: '-GNinja -DLLVM_ENABLE_PROJECTS="${{ inputs.projects }}" -DCMAKE_BUILD_TYPE=Release -DLLDB_INCLUDE_TESTS=OFF -DCMAKE_C_COMPILER_LAUNCHER=sccache -DCMAKE_CXX_COMPILER_LAUNCHER=sccache ${{ inputs.extra_cmake_args }}'
+          cmake_args: '-GNinja -DLLVM_ENABLE_PROJECTS="${{ inputs.projects }}" -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLDB_INCLUDE_TESTS=OFF -DCMAKE_C_COMPILER_LAUNCHER=sccache -DCMAKE_CXX_COMPILER_LAUNCHER=sccache ${{ inputs.extra_cmake_args }}'
           build_target: '${{ inputs.build_target }}'
 
       - name: Build and Test libclc

@sudonatalie
Copy link
Member Author

@tstellar A couple SPIR-V backend tests are XFAILed due to hitting assertions, so they're unexpectedly passing on the bot. We could change this for just SPIR-V tests but I'm presuming this is the preferred default for most others too?

@sudonatalie sudonatalie requested a review from tru December 11, 2023 15:51
@tstellar
Copy link
Collaborator

@tstellar A couple SPIR-V backend tests are XFAILed due to hitting assertions, so they're unexpectedly passing on the bot. We could change this for just SPIR-V tests but I'm presuming this is the preferred default for most others too?

I think the correct way to fix this is to add REQUIRES: asserts to the tests.

@sudonatalie
Copy link
Member Author

@tstellar A couple SPIR-V backend tests are XFAILed due to hitting assertions, so they're unexpectedly passing on the bot. We could change this for just SPIR-V tests but I'm presuming this is the preferred default for most others too?

I think the correct way to fix this is to add REQUIRES: asserts to the tests.

Ah, I'll make that change, thanks!

What do you think about enabling asserts anyways though? I would have expected that if a PR caused a passing test to hit an assert that it would cause that test to fail, and it looks like the LLVM Buildbot Infrastructure docs generally suggest "Release build types with Assertions enabled".

Copy link
Collaborator

@tstellar tstellar left a comment

Choose a reason for hiding this comment

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

I think if the buildbot guide recommends this then we should do it.

@sudonatalie sudonatalie merged commit f2afd10 into llvm:main Dec 11, 2023
5 checks passed
@sudonatalie sudonatalie deleted the enable-asserts branch December 11, 2023 18:58
sudonatalie added a commit to sudonatalie/llvm-project that referenced this pull request Dec 11, 2023
These tests currently fail on asserts, so adding a REQUIRES to make sure
they're skipped on builds with asserts disabled.

Follow-up from llvm#74849
sudonatalie added a commit that referenced this pull request Dec 14, 2023
These tests currently fail on asserts, so adding a REQUIRES to make sure
they're skipped on builds with asserts disabled.

Follow-up from #74849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants