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

#2599 silently removes some test executables and, consequently, the following tests from "Full test" stages: #2636

Open
junliume opened this issue Dec 22, 2023 · 3 comments

Comments

@junliume
Copy link
Collaborator

#2599 Silently removes some test executables and, consequently, the following tests from "Full test" stages:

  • test_immed_conv3d -all
  • test_immed_conv2d -all
  • test_pooling2d -all

Must be reverted ASAP.

@junliume @cderb @JehandadKhan

Originally posted by @atamazov in #2599 (review)

@junliume
Copy link
Collaborator Author

@atamazov I would recommend "Must be reverted fixed ASAP." to avoid too many conflicts converting CTests to GTest

@atamazov
Copy link
Contributor

@junliume The idea was to revert quickly without opening a ticket and to ask @cderb to re-implement this. This is easy to begin: create a branch from the tip of develop, revert the reverting commit (thus getting the original implementation back) and then fixing it.

Developers whose branches depend on the "single binary" PR (#2599) should avoid updating their branches from develop until the new "single binary" PR has been merged in. This way they can avoid merge conflicts.

But if you do not want to revert, then I agree. But I would say this is urgency_blocker because right now our CI does not test 2d and 3d convolutions in Immediate mode (except some solver-specific tests) and 2d pooling.

@atamazov
Copy link
Contributor

atamazov commented Dec 24, 2023

@JehandadKhan @cderb The design pattern for valid handling of test/*.cpp files can be found in #2168. Please see how it splits test/conv2d.cpp to test/conv2d.cpp and test/conv2d.hpp, and then reuses test/conv2d.hpp in some gtest.

How to "move" execution of test_* --all to gtest is another question. I think we can simply create gtest that passes the --all option to the instance of the test class. At the same time, the test executable should be added to the SKIP_TESTS list in test/CMakeLists.txt (we still need the old executable for debugging needs, but we do not want it to be run during testing). This can be done later, after this issue is fixed.

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

No branches or pull requests

4 participants