Add unit tests to see coverage increase #2267
Conversation
| @@ -1326,6 +1326,7 @@ void Study::resizeAllTimeseriesNumbers(uint n) | |||
| bindingConstraintsGroups.resizeAllTimeseriesNumbers(n); | |||
| } | |||
|
|
|||
| // TODO VP: Could be removed with the GUI | |||
There was a problem hiding this comment.
Apparently, one call to Study :: checkForFilenameLimits(...) will remain after old GUI is removed : the one inside Application :: readDataForTheStudy(...) :
if (!study.checkForFilenameLimits(true))
{
throw Error::InvalidFileName();
}So I guess the second argument will have to be removed, as it will be useless.
| @@ -1326,6 +1326,7 @@ void Study::resizeAllTimeseriesNumbers(uint n) | |||
| bindingConstraintsGroups.resizeAllTimeseriesNumbers(n); | |||
| } | |||
|
|
|||
| // TODO VP: Could be removed with the GUI | |||
| bool Study::checkForFilenameLimits(bool output, const String& chfolder) const | |||
There was a problem hiding this comment.
This function's code could really be improved.
-
As @payetvin says, some usage / calls of he function will be given up when dropping the old GUI, allowing first code simplifications (see comment above).
-
it's unclear : for example, boolean output argument actually tells the function :
- if true : to evaluate the max path length of simulation results files or , but it's not obvious at all.
- if false : not do the previous thing, but rather evaluate max path length of the input files. Not obvious.
To know that, you have to read the code carefully.
-
Function is too large : it has too many responsibilities. At least, it should be split into 2 functions ; one for input, one for output. Besides, it has 200 lines, and mixes, at the same levels, important steps and their details.
-
it's not maintainable : what if we add new input to our studies or new output to your simulation results ? We will have to modify this function each time. That's the reason why files were forgotten in this inspection.
-
It's incomplete : what about files related to short term storage, wind, ... ?
-
Lack of simplicity : for input inspection, std::filesystem::recursive_directory_iterator could have been used instead to iterate over existing files recursively and store the largest file path. the code for that would have been much shorter.
-
probably wrong : for output processing (which should remain after old GUI is removed), it's assumed that the longest file path occurs for a file value-hourly.txt somewhere in mc-all. Are we sure of that ? First, string mc-ind is longer than mc-all. Second, what about details files (for instance details-STstorage-monthly.txt) ? Aren't their path longer than value-hourly.txt paths ?
There was a problem hiding this comment.
Finally, we might be able to remove completely this function in Win by enabling long path : see this article
There was a problem hiding this comment.
The input part is not useful without the GUI, should just be removed
That leaves only the output part and thus the areaName and linkname to check
Area name is already limited to a size of 128, this leaves only linkname (which is areaname + areaname)
As it's a windows only problem it should not be handled here anyway
| @@ -291,4 +291,20 @@ BOOST_AUTO_TEST_CASE(version_parsing) | |||
| BOOST_CHECK(!v.fromString("4.5")); | |||
| BOOST_CHECK(v == StudyVersion::unknown()); | |||
| } | |||
|
|
|||
| BOOST_FIXTURE_TEST_CASE(check_filename_limit, OneAreaStudy) | |||
There was a problem hiding this comment.
Tests are normally more atomic.
Here, several things are tested in one test.
The results is that this new test has a more complex setting than it should have : it uses a fixture (which creates a study) and it creates another empty study.
This test checks 4 things.
We should create 4 smaller tests instead, one for each case, use the fixture only if a test requires it, and name these smaller tests more specifically, for clarity.
|
flomnes
dismissed
guilpier-code’s stale review
Reviewer is right, but this is slightly out of scope
No description provided.