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

Fix test file names #3881

Merged
merged 3 commits into from
Feb 19, 2025
Merged

Fix test file names #3881

merged 3 commits into from
Feb 19, 2025

Conversation

zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Feb 13, 2025

Our CI missed running tutorial because we have a filtering rule ./bin/test_*. Instead of adding another filtering rule, @xwang233 suggested that we should name all tests as test_*, which I believe makes sense.

@zasdfgbnm
Copy link
Collaborator Author

!test

Copy link

github-actions bot commented Feb 13, 2025

Review updated until commit addf428

Description

  • Rename test files to match CI filtering rule

  • Add missing buildExactGraph call in test


Changes walkthrough 📝

Relevant files
Bug fix
test_tutorial.cpp
Add missing graph build call                                                         

tests/cpp/test_tutorial.cpp

  • Added buildExactGraph call before accessing idGraph
+1/-0     
Tests
CMakeLists.txt
Rename test to match CI rule                                                         

CMakeLists.txt

  • Renamed test from nvfuser_tests to test_nvfuser
+2/-2     

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ Recommended focus areas for review

New Function Call

The new line id_model.buildExactGraph(); was added. Ensure that this function call is necessary and does not introduce any unintended side effects or performance issues.

id_model.buildExactGraph();
Test Naming

The test name was changed from nvfuser_tests to test_nvfuser. Verify that this change does not affect the CI/CD pipeline and that all relevant tests are still being executed.

add_test(test_nvfuser "${JIT_TEST_SRCS}" "")
list(APPEND TEST_BINARIES test_nvfuser)

@zasdfgbnm zasdfgbnm marked this pull request as ready for review February 13, 2025 07:27
@zasdfgbnm zasdfgbnm requested review from naoyam and xwang233 February 13, 2025 07:28
Comment on lines +657 to +658
add_test(test_nvfuser "${JIT_TEST_SRCS}" "")
list(APPEND TEST_BINARIES test_nvfuser)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Today, this is implemented as an exception in our CI filtering rule. Let's fix it, instead of making this an exception. It is annoying that after I have typed test_ and pressed tab for completion, only to remember that nvfuser tests are not starting from test_.

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@zasdfgbnm
Copy link
Collaborator Author

!test

@xwang233
Copy link
Collaborator

There seems to be some CI scripting issue and I just fixed it.

@zasdfgbnm
Copy link
Collaborator Author

!test

@zasdfgbnm
Copy link
Collaborator Author

!test

@zasdfgbnm zasdfgbnm merged commit 43f8c2a into main Feb 19, 2025
52 of 53 checks passed
@zasdfgbnm zasdfgbnm deleted the fix-tutorial branch February 19, 2025 04:16
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.

3 participants