[DRAFT] test: cover behavior gaps surfaced by external review#14
Draft
speckhard wants to merge 2 commits intoLeMaterial:mainfrom
Draft
[DRAFT] test: cover behavior gaps surfaced by external review#14speckhard wants to merge 2 commits intoLeMaterial:mainfrom
speckhard wants to merge 2 commits intoLeMaterial:mainfrom
Conversation
Pure test additions; no production code changes. Each test pins or exercises a documented public-API contract that previously had no coverage. test_database.py: - test_database_open_mmap_populate covers Database.open(populate=True), the documented Linux page-prefault path which had no smoke test. - test_database_negative_indexing_raises_index_error pins that Database[-1] is not currently supported (raises IndexError / OverflowError / ValueError) — locks in the contract until someone intentionally adds wraparound. - test_database_empty_molecule_roundtrip exercises n_atoms == 0 through the SOA encoder + parser (the empty-positions slice edge case). test_atom_molecule.py: - test_from_arrays_rejects_wrong_dtype_positions and the analogous atomic_numbers test confirm that wrong dtypes raise cleanly across the PyO3 boundary instead of silently truncating or panicking. - test_set_property_python_bool_pins_current_behavior locks in that Python bool is stored as Int(1/0) because bool is a subclass of int and PyO3 extracts i64 before f64. If anyone wants real bool storage, this test will fail loudly. test_from_ase.py: - test_from_ase_expands_voigt6_stress_to_3x3 covers the bridge's _voigt6_to_mat3x3 path; ASE's get_stress(voigt=True) returns (6,) and the bridge must expand it before storing as a 3x3 tensor.
- test_database_negative_indexing_raises_overflow_error pins the actual current exception (OverflowError from PyO3 usize extraction) and the message; previously the test caught a permissive (IndexError, OverflowError, ValueError) tuple that would have masked any future silent change. - test_database_open_mmap_populate now documents that the assertion is smoke-only on macOS (memmap2 PopulateRead advise is best-effort there); pre-faulting performance is a benchmark concern, not a unit-test one. - Add "lz4" to the four mmap-parametrized tests in test_database.py (single_item_reads_are_view_compatible, custom_array_properties_..., single_item_mutation_is_copy_on_write, single_item_pickle_...). The basic lz4+mmap=True path was covered by test_database_roundtrip[lz4] but the deeper mutation/pickle/view round-trips were not.
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
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.
Summary
Pure test additions — no production code changes. Each test pins or exercises a documented public-API contract that previously had no coverage. Surfaced by external code review.
What's tested
test_database.py(+3 tests, +1 parametrize value)test_database_open_mmap_populate—Database.open(path, mmap=True, populate=True). Smoke test only on macOS (memmap2 PopulateRead advise is best-effort there); pre-faulting performance is a benchmark concern, not a unit-test one. Documented in the test docstring.test_database_negative_indexing_raises_overflow_error— pins the actual current exception (OverflowErrorfrom PyO3'susizeextraction) and its message. If wraparound semantics are ever added, this test will fail loudly.test_database_empty_molecule_roundtrip—n_atoms == 0round-trip through the SOA encoder and parser. The 0-atom path has no upstream guard; this test exercises it end-to-end.lz4added to four mmap-parametrized tests (test_database_single_item_reads_are_view_compatible,..._custom_array_properties_roundtrip_all_numeric_tags,..._single_item_mutation_is_copy_on_write,..._single_item_pickle_materializes_owned_roundtrip). The basiclz4+mmap=Truepath was already covered bytest_database_roundtrip[lz4], but the deeper mutation/pickle/view round-trips ran only["none", "zstd"].test_atom_molecule.py(+3)test_from_arrays_rejects_wrong_dtype_positionsand..._atomic_numbers— wrong numpy dtypes (float64 positions, int64 atomic numbers) must error cleanly across the PyO3 boundary, not silently truncate or panic.test_set_property_python_bool_pins_current_behavior— locks in thatmol.set_property(key, True)storesInt(1)because Pythonboolis anintsubclass and PyO3 extractsi64beforef64. If anyone wants real bool storage, this test will fail loudly so the design choice is explicit.test_from_ase.py(+1)test_from_ase_expands_voigt6_stress_to_3x3— coversase_bridge._voigt6_to_mat3x3. ASE'sget_stress(voigt=True)returns a (6,) Voigt-form vector and the bridge must expand it to a (3,3) symmetric tensor. The test asserts both the exact element mapping and matrix symmetry.Test plan
(test_from_ase.py is gated by
pytest.importorskip("ase")like the rest of the file; the Voigt test runs only when ASE is available. Without ASE, the suite still passes — the test_from_ase module is module-level skipped.)-v).What this is not
No production code changes, no API changes, no fixture changes. Risk surface is zero — this PR can only fail by running tests against unmerged behavior changes. All assertions are against current
mainsemantics.