Fix bugs in sqlite timeseries output and add IDF parsing capabilities within unit tests

[mbadams5]

This fixes two bugs in the SQLite time series data where interval periods are incorrect or meters are missing time indexes. I added a bunch of unit tests to test upstream of the SQLite class. There are now unit tests at each of the points in OutputProcessor where it interacts with the SQLite class.

This should fix both bugs from my unit tests and manual testing, although I would like other people to test this branch as well.

I also found a bug in DetermineIndexGroupKeyFromMeterName from adding unit tests. Also, DateToStringWithMonth didn't have enough validation to pass all my new unit tests so I refactored the code after creating quite a few failing tests. That function probably never got passed invalid data so it was never an issue but now it is more robust.

I also moved the SQLiteFixture to its own header file in a 'fixtures' folder in the unit test folder. I did this since it is now used by both the SQLite.unit.cc and OutputProcessor.unit.cc

===================== Update =========================

Well this one definitely got away from me quickly.... What started out as a fairly simple bug fix turned into massive Input/Output testing and a new unit testing framework to process IDFs. This is the huge new addition. Now you can pass in an IDF snippet and have it run through the normal InputProcessor routines to fill out the global data structures. This paves the way to call GetFooInput() for any object without having to generate that global data yourself. Generating the data is error prone and can easily cause issues between different unit tests as @Nigusse and others have noticed.

So now in a unit test you use the EnergyPlusFixture (or any derived fixtures from that fixture) and this allows you to call process_idf(). You need to pass in a valid IDF snippet otherwise process_idf() will return false and may output more specific error messages as well (the same as ProcessInput() in InputProcessor). Another advantage of these fixtures is that they clear the global state of the various namespaces on TearDown() meaning at the end of every individual unit test run. This is done through a new clear_state() function in the namespace (not all namespaces have it yet, I was going through on an as need basis for now). As part of all this, the EnergyPlusFixture reads in the full Energy+.idd (that is output to the Products folder) and after the first call, will cache it. Then all subsequent calls (within that same executable run) should use this cached IDD saving time on unit testing.

I refactored some of the Humidifiers, MixedAir, and HeatBalanceManager unit tests to take advantage of this new framework and to show how to use it. There are also examples in the many new unit tests for OutputProcessor and InputProcessor as well as unit tests for my new helper classes.

The pattern i've started is that all fixtures derive from EnergyPlusFixture and you only create a new fixture if it adds needed functionality not captured in EnergyPlusFixture AND is not appropriate to add to EnergyPlusFixture. I tried to make a hierarchy and start making the fixtures adhere to OOP.

I think we should separate DeathTests from normal tests. An example of a death test is here. These are tests where you expect the program to not exit normally. In this case, ShowFatalError() is called and exits EnergyPlus. This allows us to test code paths that will cause program termination and make sure it happens how we expect.

You can now compare an expected string against the ESO, MTR, ECHO, COUT, and CERR streams as well as see if there is or is not any output on those streams.

There is a compare_containers function that will compare two different containers (either STL OR ObjexxFCL). It will make sure the two containers are the same size then iterate through each item in the container comparing the items. If they differ it will show the difference and at what index (zero based) they differ. Example usage...

EXPECT_TRUE( compare_containers( std::vector< bool >( { true } ), ObjectDef( index ).AlphaOrNumeric ) );
EXPECT_TRUE( compare_containers( Array1D_bool( { true } ), ObjectDef( index ).AlphaOrNumeric ) );

Oh and just for you @mjwitte, the EnergyPlusFixture will automatically call ShowMessage( "Begin Test: " ) on every individual unit test run, putting the test case name and test name into that message. No need to put that message into every unit test anymore.

The overall unit test coverage increased from 5.49% to 6.99%. InputProcessor.cc went from 7.0% to 58.9% and OutputProcessor.cc went from 17.3% to 65.6%.

@kbenne @macumber @asparke2 @Myoldmopar

Fixes #4979
[Fixes #97920946]

Fixes #4982
[Fixes #97921020]

[Delivers #98501064]

[mbadams5]

I may or may not add more unit tests...

[mbadams5]

@DeadParrot

On Linux/Mac, OutputProcessor::WriteRealData outputs 1.0e-2 as "1.000000000000000E-02" but on Windows it seems to output "1.000000000000000E-002". I can't seem to find a reason for this apparent platform difference. This shows up in the single failing Windows unit test. Thoughts?

[DeadParrot]

@mbadams5 This is a known platform difference. You can override it with _set_output_format(_TWO_DIGIT_EXPONENT); in an MSVC #ifdef but we'd have to make sure it can still grow to 3 digits when needed. I think Intel C++ follows MS on this but I'd have to check on current MinGW behavior. If you don't have all these compilers and want me to check into the exact behavior let me know.

Edit: Had a chance to try this out and I found that the same call is needed and works for VC, Intel, and MinGW GCC on Windows if you want 2-digit exponents when 3 aren't needed. Just putting this at the top of main seems to do the trick:

#ifdef _WIN32
    _set_output_format(_TWO_DIGIT_EXPONENT);
#endif

There's a chance some other/future Windows compiler won't support this but it is probably logical to assume most do/will.

This change could break other tests and/or alter outputs.

[mbadams5]

Humm... well I tried a different IDF and while it still has the fixes for these bugs, each of the records in the Time table are doubled. I think this is what I originally attempted to solve. I will write new unit tests to expose this behavior and attempt to fix the doubling of time entries.

[mbadams5]

Check out the updated PR text. Lots of new changes. Would like some eye balls on this.

@Myoldmopar @kbenne @mjwitte @rraustad @Nigusse

[mbadams5]

@Nigusse I changed this test to use the new input processing capabilities. The assertions are the same as you had and the only thing I changed was remove the IDD and IDF data structures creation. Instead it now uses process_idf() and the IDF snippet. Make sure this test is still what you intended, but I think it is.

[rraustad]

So process_idf DOES NOT test the input processor? Will ProcessInput eventually be replaced with this function?

[mbadams5]

process_idf does not test InputProcessor. It uses the same functions in InputProcessor however. This is really just a helper function for unit testing only. I test (most) of InputProcessor in InputProcessor.unit.cc. This new process_idf() function wraps all needed InputProcessor functions in a much more unit testing friendly way, instead of just calling InputProcessor::ProcessInput(). This new function also allows for passing only IDF snippets and more importantly, passing them as strings instead of opening IDF files.

[Myoldmopar]

@mbadams5 This is enormous, and very cool to see idf snippets working so well. From an E+ standpoint, the changes were very minor, as long as you don't randomly call clear_state(). Also from an E+ standpoint, there are no diffs --- at all. So that is wonderful.

For the original root of this pull request, I am not that familiar with the sqlite issues, but I was following this in the earlier commits as you were adding unit tests to check the output, and that those now pass. Maybe as a wrap up of this PR, and how deeply you have been with the unit tests, you could provide some guidance on whether we need (and if so, how to integrate) further SQLite testing, or your suggestions for enhancements to the unit testing framework.

I am happy to merge this, there is no reason for it to linger. We can update the team on Wednesday and maybe show off the new idf snippet based unit testing quite easily. One thing to keep in mind though. This will allow developers to call GetInput, which will provide an initial ease on unit testing some namespaces, because you can then call the other worker functions within the namespace with an index to the main array. This propagates an array-index-based EnergyPlus, which isn't the eventual path we want to be on, so guidance should definitely include both short-term and long-term unit test work.

@kbenne @mjwitte @mbadams5 I'll wait a bit today in case you have anything further, but if I don't hear anything, I'll merge in later today.

[kbenne]

This is a lot to give a reliable review. I'l start with some responses to @mbadams5's comment at the top of the PR. I wish I could line comment on comments like I can do in code.

"As part of all this, the EnergyPlusFixture reads in the full Energy+.idd (that is output to the Products folder) and after the first call, will cache it. Then all subsequent calls (within that same executable run) should use this cached IDD saving time on unit testing."

Just note that this has no value to the CI because it uses ctest which starts every test in a brand new process.

I'm not a fan of type defing the test fixture. Why not just call a spade a spade and use that as the test name? If your humidifier tests use the EnergyPlusFixture or the HVACFixture or whatever, why not just use that in the test macro for your name? You're only serving to obfuscate things, however minimally. - bike shed - because there is way too much advanced goodness going on in here for me to catch and comment on it all.

"Oh and just for you @mjwitte, the EnergyPlusFixture will automatically call ShowMessage( "Begin Test: " ) on every individual unit test run, putting the test case name and test name into that message. No need to put that message into every unit test anymore."

Isn't the test runner doing this for you anyway. This seems like unwanted noise. I'm sure @mjwitte has a good reason that I am not grasping, but maybe there are features in the test runner that people are not aware of. Like verbose output.

The ability to load idf snippets seems cool and much needed. But I understand this was implemented separately from the idf parser that EnergyPlus main routine normally uses. Hey you called this out on the phone to me @mbadams5. Why'd you do that, now I just have something to complain about. Is it technically challenging to extract this part out of energyplus so everyone can use it?

[kbenne]

I'm curious what is the analog if this if the namespaces were evolved into classes? Would you just destroy the object and recreate a new one. Or would you still have this method exposed publicly?

[kbenne]

You seem to be setting up a scenario where namespaces could become classes and perhaps their ctor take idf text or alternatively a data structure produced by your parser. ??

[kbenne]

I'm in favor of that view of the world. Data gets classified as time vary or not time varying. Class are instantiated with all not time varying data, possibly from idf parser. time varying data must be passed into and out of member functions.

[mbadams5]

I think generally most of the namespace variables become member variables of a class and, yes, get recreated on any new object. There may be cases where this needs to be publicly exposed still, but I can't think of any off the top of my head.

Yes it would be nice if each class got instantiated by a data structure parsed by the InputProcessor. I'll address your comment about 'my' parser in a different comment.

I tend to agree with your 3rd comment.

[mbadams5]

@kbenne I agree, inline commenting would be nice...

Just note that this has no value to the CI because it uses ctest which starts every test in a brand new process.

This is a general topic that needs to be covered with the team. How ctest (and thus the CI) could have subtle differences compared to straight Google Tests (aka energyplus_tests). Also, I reworked the caching so it is lazy instantiated now so it is not used EVERY fixture load. This helped even the ctest version since not all tests need to use the cached IDD.

I'm not a fan of type defing the test fixture. Why not just call a spade a spade and use that as the test name? If your humidifier tests use the EnergyPlusFixture or the HVACFixture or whatever, why not just use that in the test macro for your name? You're only serving to obfuscate things, however minimally. - bike shed - because there is way too much advanced goodness going on in here for me to catch and comment on it all.

If there is general consensus to not have the typedef'd names then i'm fine with that. I think it helps to avoid name collisions and make things more clear when you run ./energyplus_tests or ctest. But i'm ok with just calling a "spade a spade" if every wants that instead.

Isn't the test runner doing this for you anyway. This seems like unwanted noise. I'm sure @mjwitte has a good reason that I am not grasping, but maybe there are features in the test runner that people are not aware of. Like verbose output.

Kyle you are missing why @mjwitte did ShowMessage( "Begin Test: " ) to begin with. That will output the text to the eplusout.err file. I think he did that so he could look at the error file and know which lines of error file text are for which specific unit test. The error file is not an easy file (right now) to close and delete on every unit test run. So this is an intermediate step in my mind. It gets rid of all the ShowMessage() in every individual unit tests but keeps that message until the error file can be deleted every unit test.

The ability to load idf snippets seems cool and much needed. But I understand this was implemented separately from the idf parser that EnergyPlus main routine normally uses. Hey you called this out on the phone to me @mbadams5. Why'd you do that, now I just have something to complain about. Is it technically challenging to extract this part out of energyplus so everyone can use it?

This was NOT implemented separately from the idf parser that EnergyPlus main routine normally uses. Again, NOT separate. This uses the E+ functions to parse the IDF snippet and fill in the global data structures. That said, I did implement my own (naive) idf parser to pre-parse the IDF snippet before running it through the normal E+ idf parser. This was done because E+ has required-objects that must be in the IDF for it to be valid. These objects (as of now) are Building and GlobalGeometryRules. So instead of making everyone input those objects into their IDF snippets, I search their snippet for those objects. If the objects are there then I use them, and if not, then I put in a default object so the snippet is a valid IDF. That is all I use my custom IDF parser for and it is only for use within unit tests and the EnergyPlusFixture. As an aside, I originally tried to do this using a regular expression. HOWEVER, gcc has a (VERY) incomplete implementation until 4.9.0 so no matter how I tried to structure my regex it wouldn't work.

[kbenne]

so the idf parser was separate.. :)

seriously pardon my confusion.

[kbenne]

Why is there this level of indirection that seemingly does nothing but forward the call to IOHelper? Is IOHelper inaccessible (ie private) or something?

Is this specifically for comparing an FCL type against a std type? I assume if you just wanted to compare two std types you would use std::equal? I bet you are using std::equal somewhere in the implementation.

This seems like something universally useful beyond testing or io. Maybe something for FCL directly?

[mbadams5]

Yes ioHelper is private member variable. I pulled IOHelper into its own class to be able to unit test that new functionality easier. I originally had it all within EnergyPlusFixture but it was an ugly mess and not easily testable.

It can compare FCL to FCL, STL to FCL, and STL to STL. I could use std::equal to do this, yes, but I wanted to be able to check size of containers as well as output the values within the containers that differ and at what index.

There could definitely be something within E+ or FCL itself that just uses std::equal to compare any container types.

[kbenne]

One more thing and this is really the only thing that actually matters in all of my comments. @mbadams5 and @macumber, now that Mark has fixed the database bug, do we need to take action in OS to remove the workaround for this particular version of E+? Presumably that doesn't happen until we go through the OS update process, but it is important not to forget!

[macumber]

The workaround here https://github.com/NREL/OpenStudio/blob/develop/openstudiocore/src/utilities/sql/SqlFile_Impl.cpp#L2396 should only get called for E+ 8.3, when we update to 8.4 it should no longer be triggered

[Myoldmopar]

AAAAHHHH!!!! @mbadams5 what just happened?