JOSS Review - Documentation
simon-lewis opened this issue · 5 comments
JOSS Review - Documentation by @simon-lewis
Related: JOSS
Summary: Good README.md. Online documentation (Read the Docs.io @ Docs) adequate but with some limitations. No testing or contribution elements specified. The notes below refer to the Documentation @ readthedocs.io:
(1) A statement of need – Missing. Introductory paragraph on README.md is good. Otherwise covered in the Software Paper (pedantically, there is no SoN in the main documentation @readthedocs but this is not a major oversight).
(2) Installation instructions – Needs fixing. Simple PIP INSTALL. Appeared to work on container, host and windows environments but subsequent execution (of dbmsbenchmarker) failed due to unfulfilled dependencies – these would not be an issue for an experienced user but worth being more detailed here. Dependencies like JAVA and the JDBC JAR file, whilst implied, should be explicit and clear in the documentation.
(3) Example usage – Good documentation on sample usage with config examples, including simple and extended examples. Thorough.
(4) Functionality documentation – good. No particular issues (see Functionality for related elements)
(5) Automated tests – Missing. none specified.
(6) Community guidelines – Missing information on Contributing.
END.
Hi @simon-lewis, thank you for your review!
I fixed the following
- (2) included requirements concerning JAVA
- (6) link to bug tracker and contribution guidelines (good idea btw!)
I'm unsure how to provide automated tests. Without a DBMS, the tool makes no sense. Do you have any idea? What would be appropriate? A check that certain results would be expected if installed correctly? Something like exit code = 0?
I've added the following to the CONTRIBUTIONS page:
https://github.com/Beuth-Erdelt/DBMS-Benchmarker/blob/v0.12.5/docs/CONTRIBUTING.md
Testing
New features or functions will not be accepted without testing.
Likewise for any enhancement or bug fix, we require including an additional test.
If the feature or functionality concerns benchmarking
We expect it to be testable with the TPC-H workload.
Otherwise please add references to data and queries you used to test the functionality.
They must be publicly accessible.
If the feature or functionality concerns evaluation
Please include a zipped result folder, so we can trace the enhancement of evaluations based on the same results you have used.
(And please be sure to not include secret passwords in the connection information).
Great.
(1) Tests - yes, I can see this is a challenge. To be clear, what JOSS (and indeed, Github) are seeking here are standard tests which any contributor can employ to verify their code contributions. That is, how does a programmer prove they have not 'broken' DBMS-Benchmarker by changing / adding new source code ? Difficult to offer detailed advice here but my suggestion is to consider a test script / procedure that could be followed by any developer to prove the code operates completely and reliably BUT without being dependant on a specific DBMS.
- Are all timers producing results ?
- Are those results reasonable and consistent ?
- Are those results accurate ? (I can't think of a sensible way to produce parallel / independent timings to compare with the results which would prove that the timers remain accurate, though. You may know of a way. Maybe just run before-and-after code changes against a sample DB ?)
Something to think about . Perhaps take a look at other projects to see how they evolve generic test packs for their project contributors to use. The simplest is 'before-and-after' testing. That is, run the script against a test DBMS and schema before code changes, then again after the changes have been applied, and ensure the results are consistent ! This is good enough in my opinion. If you can add guidance notes on what a reasonable variance might be between runs (e.g. "timings should vary by no more than +/- 10%") then great.
(2) Contributions statement - perfect. This is what JOSS is seeking. Thanks.
Hi @simon-lewis
Thank you for the valuable hints!
I will consider adding a de-facto standard test setup beyond 'use TPC-H'. 'before-and-after' testing sounds very reasonable.
As this issue is closed, do you expect something more from my side right now before review can continue?
Hi @simon-lewis Thank you for the valuable hints! I will consider adding a de-facto standard test setup beyond 'use TPC-H'. 'before-and-after' testing sounds very reasonable.
As this issue is closed, do you expect something more from my side right now before review can continue?
All good from my perspective. The test elements are items to consider over time and I see the review as complete.