ITADN

Test suite

#423Closedjabirali 创建于 2025-09-02
J
jabiralicommented
Dear @ddahlbom, I'm currently working on the [review](https://github.com/openjournals/joss-reviews/issues/8138) for your paper submitted to JOSS. First, let me say that it's been a pleasure to have a look at your project – the code itself is impressive and I find it to be quite well documented. One point that I think can be improved a bit is the tests, which is one point on the [reviewer checklist](https://github.com/openjournals/joss-reviews/issues/8138#issuecomment-2841161349). These suggestions are not required for publication, as the tests are already more extensive and complete than most scientific code bases, but they should perhaps be considered: - **Test documentation:** While the unit tests are quite extensive, I didn't see any documentation about how to actually run the tests. I think it could be useful to add to the "Getting started" guide that e.g. running `test Sunny` from the package mode runs the test suite. This might be useful not only for developers, but also for users who want a quick check that Sunny has been installed correctly before the tutorial. - **Test commentary:** While many tests are commented (e.g. the comments in `test_ewald.jl` are decent), many have no comments at all. This makes it more difficult for someone else to determine what exactly is tested (and whether the test itself seems reasonable – since sometimes a test helps preserve backwards compatibility but doesn't test for a real issue). Some of the tests give a hint in terms of the name (e.g. "Lanczos eigenbounds" in `test_kpm.jl`), but other tests can only be understood by analyzing external files (e.g. `test_nxs.jl` seems to test that HDF5 data loading works by checking for hardcoded values in a binary asset, but this has to be deduced from context). Perhaps it could be useful to add a bit more commentary – e.g. one line per `@testitem` describing its purpose? (For large test suites it might also be useful to highlight which ones are "unit tests" and which are "integration tests", but that's not important.) - **Test coverage:** I didn't manage to check the test coverage via the terminal (using `Coverage.jl` generated some errors on MacOS), but I got it working when I tried the "Run Tests with Coverage" button in VSCode. While most of the code base seems very well-tested, the coverage reports suggest that some of the functionality is perhaps not. Notably, `PhaseAveraging.jl` has an untested function `error_basis_reduction`, `chebyshev.jl` is mostly untested (even though e.g. `apply_jackson_kernel!` is very simple it might be worth testing that the coefficients are scaled by factors between `[0, 1]` that monotonically decrease with coefficient index for example), and there are other files with many untested functions like `SymOp.jl` / `MSymOp.jl`, `EntangledSpinWaveTheory.jl`, and `Binning.jl`. It's not necessary to aim for 100% coverage, but it might be worth reconsidering whether the untested functions are important enough to deserve tests.
关闭于 2025-11-23 5 条评论