aodn / imos-toolbox

Graphical tool for QC'ing and NetCDF'ing oceanographic datasets
GNU General Public License v3.0
46 stars 31 forks source link

inc git-lfs attributes, testfolder & files #574

Closed ocehugo closed 5 years ago

ocehugo commented 5 years ago

First test for git-lfs:

  1. Include older Starmon mini files - dubbed v000
  2. Include older Starmon oddi files - idem
  3. Include newer Starmon mini files - dubbed v001
petejan commented 5 years ago

For me the testing structure needs defining first.

The data that is used as part of a given test belongs in the test directory.

I would have preferred test to be numbered as then the order in which they were developed is obvious, and automating running the test for regression testing is easy, they can be run in order without needing to add them to a list of test.

As new files are needed to be included for testing, a new test is created so that the old code fails, then a change including the new test along with its test data is created and committed together.

ocehugo commented 5 years ago

For me the testing structure needs defining first.

This PR is just about where to hold the files. I already set up how the test infrastructure will be - it follows modern standards and what can be done with Matlab test suite. You can comment on when the PR is posted (or see below for a quick description).

The data that is used as part of a given test belongs in the test directory.

The data should not be within the same directory of the source code or the test code. Data is data, code is code.

I would have preferred test to be numbered as then the order in which they were developed is obvious, and automating running the test for regression testing is easy, they can be run in order without needing to add them to a list of test.

Numbering tests is bad practice/architecture, as well as running tests in order. Tests should run in any order. If you need a particular ordering, you create a particular test that will do that. Tests should also have meaningful names, eg "testReadStarmonMini, testContentStarmonMini,testStarmonWithWrongLineDef". I'm not a fan of camelCase, but I'm keeping with the flow here.

Hopefully, Matlab implemented parametrised tests if you use testCase classes. This enables the testing framework with way less boilerplate (faster to code/modify), is more robust, and also more performant.

As new files are needed to be included for testing, a new test is created so that the old code fails, then a change including the new test along with its test data is created and committed together.

Yes. This is the way I work but not the way it has been done. However, this also required some tooling around to be feasible - say compare content after reading, not just test if a read is successful. Content comparison is straight forward (isequal), but tracing down what was change is the actual important bit that helps fix problems. The current inner structure stored in the toolbox is highly variable (e.g. meta fields). Finally, the implemented tools should also abide by the same rules (testing).

It is essential is to create these things NOW so regressions/bugs/enhancements/new features can be coded with confidence and without too much fidgeting.

At the moment, without tests, the line is thin.

Quick description of the test structure for the parser/reader is :

  1. A class/function specific for the instrument reader.
  2. Test-ReadFile should read instrument file(s) successfully.
  3. Test-Content should read the instrument file(s), and content should be matched against a validated content.
  4. Test-A,B,C,D,E,F,etc are specifics or new things along the way (version changes, problems detected, etc).

I'm avoiding as much as possible class boilerplate, setUp/tearDown cases and ordering. This is usually not required in the simple, functional style of the toolbox. Moreover, no historical checking is needed. Once the test runtime is setup, we will always be able to track and write specific tests.

Example: I started working on your starmonmini issue. Test-ReadFile and Test-Content is a success for all older versions (v000) but both failed for v001 folder. Your starmonmini file required a new test. The new test was written, and I changed the code until all tests pass (new PR coming soon).

Test-Content is a success for all v000 files since it compares what is read with a mat file created with the current tag of the toolbox. This test check for type, the structure of the struct, and content of each entity. Although "content regressions" can still be hiding in there (the content generated by the current tag is not what it should be), the machinery is working.

ocehugo commented 5 years ago

sorry I did a wrong merge here - the branch was in bad state. will send a new branch again.