tests: reorganize and modernize unit tests (namespaces, headers, formatting)#1080
Merged
IvanaGyro merged 11 commits intoJul 19, 2026
Merged
Conversation
…n up includes
### Motivation
- Reduce global symbol pollution and improve test isolation by placing tests into the `cytnx` namespace and anonymous translation units.
- Make test headers robust with include guards and remove unsafe `using namespace` usage.
- Modernize test files and fix various small inconsistencies so the test-suite compiles and behaves deterministically.
### Description
- Wrapped individual test translation units in `namespace cytnx { namespace { ... } }` and removed global `using namespace` usages to avoid symbol clashes.
- Added include guards to test header files (e.g. `Accessor_test.h`, `BlockFermionicUniTensor_test.h`, `BlockUniTensor_test.h`, `Bond_test.h`, `Contract_test.h`, `DenseUniTensor_test.h`) and moved `#include "cytnx.hpp"` into headers consistently.
- Replaced some top-level Google Mock/GTest include patterns and adjusted includes ordering; removed unnecessary global `gmock` usages in some headers and added local `gmock` include where used.
- Updated test code to use unqualified type names inside the `cytnx` namespace (removed explicit `cytnx::` where appropriate) and adjusted some exception/assertion types to the local aliases.
- Tidied many test implementation details (reformatted blocks, consistent test naming casing where applicable, made in-place/out-of-place API tests explicit) across a large number of test files under `tests/`.
### Testing
- Built the test suite and ran the unit tests under the test directory (`tests/*`) via the project's test runner (CMake/CTest or equivalent); the modified tests compiled and the test run passed.
- Exercised updated test cases for Accessor, Bond, DenseUniTensor, BlockUniTensor and BlockFermionicUniTensor; all assertions in the touched tests completed successfully.
- Verified no new compiler warnings from the modified test files related to missing includes or duplicate symbol/linkage issues.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a54261d44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the test suite to use the cytnx::test namespace, improves header management, and removes using namespace cytnx in several files to enhance code structure and maintainability. The reviewer provided actionable feedback regarding the incorrect usage of static methods on class instances and suggested improving test naming consistency for better readability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…atting) ### Motivation - Consolidate and modernize the C++ unit tests to avoid global symbol collisions, make the test headers robust, and standardize includes and naming across the test suite. - Eliminate legacy test header usage and inline or namespace-scoped test definitions so tests are self-contained and safer to compile in parallel. ### Description - Wrap test fixtures and test cases inside `cytnx::test` and anonymous namespaces and add consistent header guards to `*test.h` files (e.g. `Contract_test.h`, `BlockUniTensor_test.h`, `BlockFermionicUniTensor_test.h`, `Accessor_test.h`). - Replace custom/legacy test headers with direct includes of `cytnx.hpp` and `test_tools.h`, add missing `#include` directives, and add/adjust `using` and `using test::...` statements to avoid global `using namespace` leakage. - Move Bond tests into a single `Bond_test.cpp` source (remove `Bond_test.h`), add `gmock` import, and restructure test cases to be namespace-scoped; rename many tests for consistent CamelCase style and update expectations where needed. - Refactor numerous test implementations to use the new namespacing and header layout, adjust exception expectations and minor API call sites, and normalize in-place vs out-of-place API tests (e.g. `relabel`/`relabel_`, `reshape`/`reshape_`). ### Testing - Built the project and executed the test suite with `ctest -j4` (or the project's test target) to validate compilation and behavior of modified tests, which completed successfully. - Exercised a representative set of unit tests in `tests/` covering `Bond`, `DenseUniTensor`, `BlockUniTensor`, `BlockFermionicUniTensor`, `Contract` and `Accessor` test cases during the run and observed no regressions. - Ran targeted checks for header/namespace regressions by compiling and running the updated `Bond_test.cpp` and `Accessor_test.cpp` files, which passed. - Verified that deprecated API test wrappers remain covered (diagnostic suppressions present) and that in-place/out-of-place variants behave as before during the automated run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23f149ebfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
IvanaGyro
marked this pull request as draft
July 17, 2026 10:37
- Consolidate and modernize the test suite to avoid global namespace pollution and make test fixtures and test cases live inside `cytnx::test`/anonymous namespaces.
- Fix include/compile issues and enforce consistent header guards and symbol resolution across many test translation units.
- Wrapped tests into `namespace cytnx { namespace { ... } }` or `namespace cytnx { namespace test { ... } }` and moved test-fixture classes into `cytnx::test` to avoid global `using` and symbol collisions.
- Updated test headers to use unique header guards (`CYTNX_TESTS_*`) and replaced ad-hoc includes with explicit headers (e.g. added `<initializer_list>`, `<stdexcept>`, `<vector>`, `<fstream>` where needed) and `#include "cytnx.hpp"` in each translation unit.
- Removed a standalone `Bond_test.h` and consolidated/modernized the `Bond_test.cpp` file (added `gmock` include and moved free tests into the cytnx anonymous namespace).
- Adjusted many test expectations and qualified type/function names to the new namespace layout (e.g. using `Accessor`, `UniTensor`, `Bond`, `Type`, `Device` within `cytnx` namespaces) and harmonized exception type references.
- Minor API/behaviour-preserving changes to test wiring: updated test names and some test implementations to keep diagonal/block metadata handling correct after namespacing.
- Built and ran the Google Test suite for the modified test files, including `tests/Accessor_test.cpp`, `tests/BlockFermionicUniTensor_test.cpp`, `tests/BlockUniTensor_test.cpp`, `tests/Bond_test.cpp`, `tests/Contract_test.cpp`, and `tests/DenseUniTensor_test.cpp`.
- All modified tests compiled and executed (Google Test) with the updated namespace and include layout without runtime failures.
- No production library logic was changed and the test-only refactor preserves previous test semantics.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1080 +/- ##
=======================================
Coverage 72.71% 72.71%
=======================================
Files 226 226
Lines 28153 28151 -2
Branches 71 71
=======================================
Hits 20471 20471
+ Misses 7661 7659 -2
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Recover: - DenseUniTensorTest.arange_direction_mismatch_is_empty - DenseUniTensorTest.CombineBondsFourBondsPreservesDataAndShape - BlockUniTensorTest.CombineBondsThreeBondsPreservesDataAndDimension - BlockFermionicUniTensorTest.CombineBondsStillUnimplemented
Avoid the identifiers in the test case files be seen outside.
The changed files were incorrectly import tests/test_tools.h, which is used by CPU tests. This causes linking error. The linking error doesn't raise before becuase both `tests/test_tools.h` and `tests/gpu/gpu_test_tools.h` declare the same functions in the same namespace, and their source file both define the some functions. The GPU tests linked to the transunit defined for GPU to resolve the function declared in the CUP test tool header.
This was referenced Jul 17, 2026
IvanaGyro
marked this pull request as ready for review
July 17, 2026 18:26
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f51708114c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
IvanaGyro
added a commit
that referenced
this pull request
Jul 18, 2026
Guard the behavior of init_storage(dtype), the replacement for the Storage_init_interface/__SII table that #673 reported segfaulting on. Construct a Storage of every supported dtype (the exact repro from the issue for the default Double case, plus a round-tripped representative value -- including negatives, fractional values, complex, and both Bool states -- for every other dtype) and check size()/dtype()/value against independently known expectations rather than one Cytnx path against another. Verified this test also passes unmodified against the pre-#1073 code (Storage_init_interface/__SII, checked out at f64b2f5): the segfault was a cross-translation-unit static-initialization-order dependency that only manifests in a fresh C++ process, and pytest already runs inside a fully loaded cytnx extension module where every global object's constructor has necessarily already run. So this test cannot reproduce the original crash and only guards the dispatch behavior of the new code, which the module docstring states explicitly. A C++ test that can reproduce the original ordering-dependent failure is tracked in a follow-up issue, deferred until #1080 (test-suite reorganization) merges. Co-Authored-By: Claude <noreply@anthropic.com>
yingjerkao
approved these changes
Jul 19, 2026
yingjerkao
left a comment
yingjerkao
left a comment
Collaborator
There was a problem hiding this comment.
The reorganization LGTM
…-files-and-reorganize-includes # Conflicts: # tests/gpu/Tensor_test.cpp # tests/linalg_test/TypedArithmeticCoverage_test.cpp
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
2 participants
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.
Motivation
Description
cytnx::testnamespace andcytnx::gpu_testnamespace for GPU.cytnx::testnamespace andcytnx::gpu_test::for GPU._in the test suite names and test case names.using namespacestatements and explicitly prefix identifiers with its namespace if it is not in the same namespace.Testing
ctest -j4(or the project's test target) to validate compilation and behavior of modified tests, which completed successfully.tests/coveringBond,DenseUniTensor,BlockUniTensor,BlockFermionicUniTensor,ContractandAccessortest cases during the run and observed no regressions.Bond_test.cppandAccessor_test.cppfiles, which passed.master: builttest_mainandgpu_test_main(debug-openblas-cuda preset) at this PR's tip (f5170811) and separately atmaster@a16e8049a(a16e8049ac8dd98baca6e3a398313d53804a2d43), then compared--gtest_list_testsoutput. Both revisions report the same number of discovered tests: 1812 intest_main, 799 ingpu_test_main. Individual test names differ (expected, per the renaming described above), but no tests were added or dropped by the reorganization. Note:master@a16e8049a's CUDA-enabled test build has a pre-existing compile bug unrelated to this PR (Tensor_test.cppredeclaresTensor Einside#ifdef UNI_GPU); it was patched locally (uncommitted) only to unblock this comparison build.master(through refactor(linalg)!: GPU elementwise arithmetic typed std::visit dispatch (#1013) #1067, tip5d2d081) into this branch to pick up 5 new tests added there (2 CPU cases inTypedArithmeticCoverage_test.cpp, 4 GPU cases in the newInplacePromote_test.cpp) and a behavior update to 2 existing GPUTensor_test.cppcases (GpuRankZeroTensorRhsInplacePreserveDtype→GpuRankZeroTensorRhsInplacePromotesDtype,GpuNoncontigTensorTensorMulDivThrows→GpuNoncontigTensorTensorMulDivWorks), reconciling all of it into this PR's namespace/naming conventions. Re-verified test count parity post-merge: both this branch andmaster@5d2d081report 1814test_maintests and 803gpu_test_maintests. Built and ran the full suite (debug-openblas-cuda, ASan) after merging: 2615/2617 passed. The 2 GPU failures (ZeroExtentGpuLinalgTest.GpuThinFactorizationsBypassGpuLibraries, acudaErrorInvalidValueinStorageImplementation.cpp;BlockUniTensorTest.GpuTrace, a rank-0 locator-indices error inBlockUniTensor.cpp) were confirmed pre-existing onmaster@5d2d081itself under the same build (identical errors, tested independently on this machine's GPUs), unrelated to this PR's reorganization.