HPWH Equipment Sequence Number Error to Warning #5124

nmerket · 2015-08-19T14:46:43Z

Todo:

Unit test

nmerket · 2015-08-19T18:13:27Z

Okay, I know I'm asking something that is sacrilege around here, but can I not do a unit test for this? The only change was to alter the text of an error message so it is more clear and turn that error into a warning. I'm not sure adding a unit test adds value here.
@Myoldmopar @mbadams5

nmerket · 2015-08-20T19:07:50Z

@Myoldmopar you never answered my question: Is this okay without a unit test?

Myoldmopar · 2015-08-20T19:37:07Z

So....it is quite fair to say that if it is literally just a typo in a string or such that it does not need a unit test. Your change does slightly more than that though. You don't trip the ErrorsFound flag anymore, which does change the functionality of the program, not just the error message. So, in general, I would say that this does require a unit test to verify that.

How about changing that function from a void to a bool. Then at the end of the function, just return the value of ErrorsFound. This shouldn't interfere with other callers of the function, since they would have been expecting a void, and it can verify that with prescribed inputs, that function does pass through successfully now.

(hindsight) From a test-driven development perspective, it probably would have been wise to change the function to a bool return first, set up the unit test to call that function, and given the those inputs, the test would have failed with a false return value. Then with your fix, you get the unit test to pass with a true return value. That sounds ideal. 😉

nmerket · 2015-08-20T19:50:15Z

@Myoldmopar Right, it is a little backwards for test driven development. Sorry. The problem is that if ErrorsFound == true, then GetWaterThermalTankInput dies before completing. So, either it returns false if everything went okay or it dies by way of ShowFatalError, there is no way to get a true return value from that function without refactoring things even more.

Myoldmopar · 2015-08-20T19:51:51Z

Inside GetWaterThermalTankInput:

return ErrorsFound;

Inside your unit test:

expect_false(GetWaterThermalTankInput());

Right?

Myoldmopar · 2015-08-20T19:53:21Z

Because then you could also test expected failures in input processing just by providing the bad input idf snippet and using expect_true.

nmerket · 2015-08-20T20:01:23Z

The problem is it does this several times in GetWaterThermalTankInput before it's done. To make that work, I'd have to change all of those to return true; and then any time GetWaterThermalTankInput is called look for that and throw a fatal error if it's true.

Myoldmopar · 2015-08-20T20:04:02Z

OK, fair. So we won't try to trap for the bad cases anytime soon. But you should be able to check for a successful run with the expect_false I mentioned before. If you give it valid inputs, and get input processes it successfully, the code makes it all the way through the function, so it will return a false value of ErrorsFound.

nmerket · 2015-08-20T20:09:10Z

Okay. I'll take a crack at a unit test then.

Myoldmopar · 2015-08-20T20:10:28Z

Thanks @nmerket. If it was literally just the string change, it would be easier to justify. But it shouldn't take long to set this unit test up. Just the idf snippet, process_idf, and expect_false.

nmerket · 2015-08-21T17:07:29Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

+	EXPECT_FALSE( WaterThermalTanks::GetWaterThermalTankInput() );
+}
+
+TEST_F( HVACFixture, HPWHWrappedDummyNodeConfig )


Merging in this other unit test from develop caused me some problems. It's failing now because the state isn't being cleared for water heaters. @EnergyArchmage is there a implementation of clear_state for WaterThermalTanks in that branch you're maintaining? I would have looked myself, but @Myoldmopar didn't mention the branch name in the email so I thought it would be easier to just ask you.

nmerket · 2015-08-21T17:11:38Z

@Myoldmopar @mbadams5 The unit test I wrote for this works, but it's not reading the error message to make sure it comes out right. I tried for a bit to create another test fixture that combined HVACFixture and SQLiteFixture using multiple inheritance, but it fell apart quickly, so I abandoned the effort. Hope that's okay.

… work again.

nmerket · 2015-08-28T15:17:48Z

I think this is ready. It's been like whack-a-mole for something that should have been really simple. In my debug build in Xcode, energyplus_test runs fine. However, when I do a release build on my mac with makefiles, it segfaults at HVACFixture.VRFTest. Maybe I need to run cmake again. Either way. I'm hoping the CI machines catch it if there's something wrong.
@mbadams5 Can you check this out and make sure I didn't do something really stupid?

nmerket · 2015-08-28T16:41:53Z

Okay, I just re-ran cmake on my xcode build and it picked up that test. It's an array out of bounds assertion error.
Wait a minute. The HVACFixture.VRFTest is causing the same error on develop too. This isn't my fault! Yay!
@Myoldmopar @mbadams5

mjwitte · 2015-08-28T19:05:28Z

@nmerket Interesting. That test HVACFixture.VRFTest was passing over in #5135
FYI @EnergyArchmage @rraustad

rraustad · 2015-08-28T19:12:50Z

There are still issues with clear_state in some modules. Working on it, but it's a snails pace.

nmerket · 2015-08-28T19:24:28Z

@mjwitte @rraustad Yeah, as you can see the CI machines don't seem to catch this, but when I run energyplus_tests on a clean build of develop I hit it.

[ RUN      ] mHVACFixture.VRFTest
Assertion failed: (contains( i )), function operator(), file /Users/nmerket/energyplus/gitrepo/third_party/ObjexxFCL/src/ObjexxFCL/Array1.hh, line 1236.

mjwitte · 2015-08-28T19:29:32Z

@nmerket The CI machines run the tests individually that's why it's different. When you energyplus_tests is runs them serially, so that's why it can turn out differently. If you filter just that test it will probably run OK on your local machine. --gtest_filter=*VRFTest

nmerket · 2015-08-28T19:49:14Z

So, if that's the requirement--each test can be run individually, but not necessarily all together--then this is good to go pending a favorable CI result.

…95_hpwh_equip_seq_num Conflicts: tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

mbadams5 · 2015-08-28T20:07:05Z

@nmerket I just updated this also with some more changes.

As an aside, you can remove tests from the local testing using --gtest_filter

energyplus_tests --gtest_filter=-*VRFTest*

That will run all tests except ones that match *VRFTest*. The - is the key part.

nmerket · 2015-08-28T20:13:45Z

@mbadams5 Thank you for figuring out the clear_state mess. I guess I did comment out that VRFTest, sorry.

mbadams5 · 2015-08-28T20:16:29Z

@nmerket The VRFTest still fails for me locally, but I don't think that is due to this branch.

nmerket · 2015-09-02T20:24:53Z

Just merged develop back into this branch and did some clean up. Unit tests are all working. Should be ready to merge. @Myoldmopar do I change the assignee to you?

Myoldmopar · 2015-09-02T20:25:48Z

Feel free to; I'll take a look with the other bugs after the new features are wrapped up this week.

…equip_seq_num Conflicts: tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc tst/EnergyPlus/unit/Fixtures/HVACFixture.hh

mjwitte · 2015-09-08T18:59:06Z

@nmerket I've merged in develop, resolved conflicts, then fixed a couple of things. Not sure if git got confused or I just didn't to the conflict resolution correctly. In any case, please review the diffs to be sure it looks correct. If CI comes back OK, then this can go in.

mjwitte · 2015-09-08T19:13:41Z

@nmerket I already see some mistakes - will update one more time.

mjwitte · 2015-09-08T19:42:35Z

@nmerket That's better. Now ready for your review.

nmerket · 2015-09-09T18:36:20Z

@mjwitte Looks good to me. Should I press the big, green button or should you? Or should @Myoldmopar? I never know the protocol.

mjwitte · 2015-09-09T19:11:49Z

@nmerket I will. It's considered bad form to merge your own pull request. One rare occasions, maybe.

HPWH Equipment Sequence Number Error to Warning

nmerket added 2 commits August 19, 2015 08:40

changing error message to warning and rewording #4695

9de9ed7

changing to warning error

81ee427

nmerket self-assigned this Aug 19, 2015

nmerket added 2 commits August 21, 2015 10:50

adding working unit test

840f815

merging in develop, problems in water heater unit tests

f77bdba

nmerket reviewed Aug 21, 2015
View reviewed changes

Myoldmopar added the Defect Includes code to repair a defect in EnergyPlus label Aug 25, 2015

nmerket added 2 commits August 28, 2015 08:38

adding WaterThermalTanksFixture and a bunch of clear_state functions.…

cdd7a79

… work again.

Merge branch 'develop' into 4695_hpwh_equip_seq_num

93140a8

removing WaterThermalTankFixture

1fbba86

mbadams5 added 2 commits August 28, 2015 15:58

Update WaterThermalTanks unit tests

1184fe5

Merge in changes

517ae8a

mbadams5 added 2 commits August 28, 2015 16:03

Merge remote-tracking branch 'origin/4695_hpwh_equip_seq_num' into 46…

9089846

…95_hpwh_equip_seq_num Conflicts: tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

Fix merge issues

5cb7d08

Fix another merge issue

7f502ff

nmerket added 3 commits September 2, 2015 14:08

merging in develop and resolving conflicts

a0cbfee

removing redundant clear_states from HVACFixture

e9232b6

removing extra clear_state from header

cb971b5

nmerket assigned Myoldmopar and unassigned nmerket Sep 2, 2015

mjwitte added 2 commits September 8, 2015 13:12

Merge remote-tracking branch 'remotes/origin/develop' into 4695_hpwh_…

054c657

…equip_seq_num Conflicts: tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc tst/EnergyPlus/unit/Fixtures/HVACFixture.hh

HPWH Equipment sequence - fix bad merge conflict resolution

eb809bd

PWH Equipment sequence - fix bad merge conflict resolution - again

adc9167

mjwitte added a commit that referenced this pull request Sep 9, 2015

Merge pull request #5124 from NREL/4695_hpwh_equip_seq_num

4ecbf49

HPWH Equipment Sequence Number Error to Warning

mjwitte merged commit 4ecbf49 into develop Sep 9, 2015

mjwitte deleted the 4695_hpwh_equip_seq_num branch September 9, 2015 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPWH Equipment Sequence Number Error to Warning #5124

HPWH Equipment Sequence Number Error to Warning #5124

nmerket commented Aug 19, 2015

nmerket commented Aug 19, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket Aug 21, 2015

nmerket commented Aug 21, 2015

nmerket commented Aug 28, 2015

nmerket commented Aug 28, 2015

mjwitte commented Aug 28, 2015

rraustad commented Aug 28, 2015

nmerket commented Aug 28, 2015

mjwitte commented Aug 28, 2015

nmerket commented Aug 28, 2015

mbadams5 commented Aug 28, 2015

nmerket commented Aug 28, 2015

mbadams5 commented Aug 28, 2015

nmerket commented Sep 2, 2015

Myoldmopar commented Sep 2, 2015

mjwitte commented Sep 8, 2015

mjwitte commented Sep 8, 2015

mjwitte commented Sep 8, 2015

nmerket commented Sep 9, 2015

mjwitte commented Sep 9, 2015

HPWH Equipment Sequence Number Error to Warning #5124

HPWH Equipment Sequence Number Error to Warning #5124

Conversation

nmerket commented Aug 19, 2015

nmerket commented Aug 19, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket commented Aug 20, 2015

Myoldmopar commented Aug 20, 2015

nmerket Aug 21, 2015

Choose a reason for hiding this comment

nmerket commented Aug 21, 2015

nmerket commented Aug 28, 2015

nmerket commented Aug 28, 2015

mjwitte commented Aug 28, 2015

rraustad commented Aug 28, 2015

nmerket commented Aug 28, 2015

mjwitte commented Aug 28, 2015

nmerket commented Aug 28, 2015

mbadams5 commented Aug 28, 2015

nmerket commented Aug 28, 2015

mbadams5 commented Aug 28, 2015

nmerket commented Sep 2, 2015

Myoldmopar commented Sep 2, 2015

mjwitte commented Sep 8, 2015

mjwitte commented Sep 8, 2015

mjwitte commented Sep 8, 2015

nmerket commented Sep 9, 2015

mjwitte commented Sep 9, 2015