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

Add check-clad-execonly target #875

Merged
merged 1 commit into from
May 29, 2024
Merged

Conversation

parth-07
Copy link
Collaborator

@parth-07 parth-07 commented Apr 28, 2024

This commit adds a new verification target, 'check-clad-execonly'. This target only verifies the 'CHECK-EXEC' filecheck checks. It is helpful during the development phase when derivative code is expected to change frequently.

Fixes #874

@parth-07 parth-07 marked this pull request as draft April 28, 2024 08:14
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link

codecov bot commented Apr 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.09%. Comparing base (e3c1ff0) to head (1d8f8d6).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #875   +/-   ##
=======================================
  Coverage   94.09%   94.09%           
=======================================
  Files          53       53           
  Lines        7805     7805           
=======================================
  Hits         7344     7344           
  Misses        461      461           

@parth-07 parth-07 marked this pull request as ready for review April 28, 2024 08:37
test/lit.cfg Outdated Show resolved Hide resolved
Copy link
Owner

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

That's quite handy. Thank you! I bet @PetroZarytskyi will appreciate that maybe more than anybody else ;)

@@ -24,6 +24,8 @@ if(CLAD_BUILT_STANDALONE)
set(LLVM_LIBS_DIR ${LLVM_INSTALL_PREFIX}/lib/)
endif()

set(CLAD_FILECHECK_CHECK_EXEC_ONLY False)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need a separate option here? Can't we just dial the target with make check-clad-exec-only?

Copy link
Collaborator Author

@parth-07 parth-07 Apr 29, 2024

Choose a reason for hiding this comment

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

This option is used for configuring lit.cfg for the two cases -- check derivative code and don't check derivative code. Now we have two lit.site.cfg files that represents these two cases: test/lit.site.cfg and test/CheckExecOnly/lit.site.cfg.

This option is used so that we set the correct value of config.filecheck_check_exec_only when we call configure_lit_site_cfg.

set(CLAD_FILECHECK_CHECK_EXEC_ONLY True)
configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in
${CMAKE_CURRENT_BINARY_DIR}/CheckExecOnly/lit.site.cfg
Copy link
Owner

Choose a reason for hiding this comment

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

Can we get away without creating a new folder? I suspect that you could pass an extra configuration to lit. Using lit --debug might be able to hint you how to do it...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems interesting. I will look into it.

Copy link
Owner

Choose a reason for hiding this comment

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

@parth-07, I've pushed some changes which make the implementation a bit more flexible. Can you take a look?

@@ -74,6 +82,14 @@ add_lit_testsuite(check-clad "Running the Clad regression tests"
)
set_target_properties(check-clad PROPERTIES FOLDER "Clad tests")

add_lit_testsuite(check-clad-exec-only "Running the Clad regression tests"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
add_lit_testsuite(check-clad-exec-only "Running the Clad regression tests"
add_lit_testsuite(check-clad-exec-only "Running the Clad execution regression tests"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev vgvassilev changed the title Add check-clad-exec-only target Add check-clad-execonly target May 29, 2024
This commit adds a new verification target, 'check-clad-exec-only'.
This target only verifies the 'CHECK-EXEC' filecheck checks. It is
helpful during the development phase when derivative code is expected to
change frequently.

Co-authored-by: Vassil Vassilev <[email protected]>

Fixes vgvassilev#874
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Collaborator Author

@parth-07 parth-07 left a comment

Choose a reason for hiding this comment

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

Looks good to me. @vgvassilev Thank you for the changes. Your changes made the functionality much more flexible and clean.

@vgvassilev vgvassilev merged commit 068e6b4 into vgvassilev:master May 29, 2024
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a target for verifying only CHECK-EXEC in tests
3 participants