Remove or replace compare_err_stream_substring when an empty string is being compared

[mitchute]

Pull request overview

The behavior of compare_err_stream_substring appears to have been misunderstood, (propagated via copy/paste,) and possibly allowed errors to pass by uncaught. For std::string::find(""), C++ considers the empty string found at position 0 for any string, including another empty string. See the MWE, here.

This makes usages like compare_err_stream_substring("", true) ineffective at doing anything other than clearing the err stream. If the intent is really to expect an empty err stream, we should be using compare_err_stream instead.

This PR replaces those usages, and adds guards to protect against further misuse.

Description of the purpose of this PR

Pull Request Author

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies
• If adding/removing any output files (e.g., eplustbl.*)
  • Update ..\scripts\Epl-run.bat
  • Update ..\scripts\RunEPlus.bat
  • Update ..\src\EPLaunch\ MainModule.bas, epl-ui.frm, and epl.vbp (VersionComments)
  • Update ...github\workflows\energyplus.py

Reviewer

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[mitchute]

Locally, I'm still seeing the following as a result of these changes:

1649: [ RUN      ] EnergyPlusFixture.OutdoorAirUnit_AutoSize
1649: /home/mitchute/Projects/EnergyPlus/tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc:223: Failure
1649: Expected equality of these values:
1649:   expected_string
1649:     Which is: ""
1649:   stream_str
1649:     Which is: "   ** Warning ** SizeDXCoil Coil:Cooling:DX:SingleSpeed ACDXCOIL 1\n   **   ~~~   ** ...Gross Rated Total Cooling Capacity [W] will be limited by the maximum rated volume flow per rated total capacity ratio.\n   **   ~~~   ** ...DX coil volume flow rate [m3/s] = 0.500000\n   **   ~~~   ** ...Requested capacity [W] = 75.000\n   **   ~~~   ** ...Requested flow/capacity ratio [m3/s/W] = 0.00666667\n   **   ~~~   ** ...Maximum flow/capacity ratio [m3/s/W] = 6.04100E-05\n   **   ~~~   ** ...Adjusted capacity [W] = 8276.775\n   ** Warning ** SizeHeatingCoil: : Potential issue with equipment sizing for Coil:Heating:Electric ZONE1OAUHEATINGCOIL\n   **   ~~~   ** ...Rated Total Heating Capacity = 0.00 [W]\n   **   ~~~   ** ...Capacity used to size child component set to 0 [W]\n"
1649: With diff:
1649: @@ -1,1 +1,10 @@
1649: -""
1649: +   ** Warning ** SizeDXCoil Coil:Cooling:DX:SingleSpeed ACDXCOIL 1
1649: +   **   ~~~   ** ...Gross Rated Total Cooling Capacity [W] will be limited by the maximum rated volume flow per rated total capacity ratio.
1649: +   **   ~~~   ** ...DX coil volume flow rate [m3/s] = 0.500000
1649: +   **   ~~~   ** ...Requested capacity [W] = 75.000
1649: +   **   ~~~   ** ...Requested flow/capacity ratio [m3/s/W] = 0.00666667
1649: +   **   ~~~   ** ...Maximum flow/capacity ratio [m3/s/W] = 6.04100E-05
1649: +   **   ~~~   ** ...Adjusted capacity [W] = 8276.775
1649: +   ** Warning ** SizeHeatingCoil: : Potential issue with equipment sizing for Coil:Heating:Electric ZONE1OAUHEATINGCOIL
1649: +   **   ~~~   ** ...Rated Total Heating Capacity = 0.00 [W]
1649: +   **   ~~~   ** ...Capacity used to size child component set to 0 [W]\n
1649: 
1/1 Test #1649: EnergyPlusFixture.OutdoorAirUnit_AutoSize ...SIGTRAP***Exception:   1.23 sec

@rraustad do you have thoughts on this one?

[rraustad]

@mitchute changing from what you describe as a useless compare_err_stream_substring("", true); (I wasn't sure what that was doing anyway) to an actual check of the error string using compare_err_stream("", true); I would expect that there would be phantom errors showing up here and there in unit tests. I guess I would delete that error check or change "" to something more informative (e.g., "Gross Rated Total Cooling Capacity [W] will be limited by the maximum rated volume flow per rated total capacity ratio").

[rraustad]

I do like this though! Now we can't be lazy.

[mitchute]

@rraustad the error I show above is a phantom error that was being thrown and hidden, but is now appearing. It's occurring due to this line being removed:

https://github.com/NatLabRockies/EnergyPlus/pull/11648/changes#diff-5df7d341b7cc0593a8f16b92812f16290e3e83e9961a3030960cae9b8fa35582L364

compare_err_stream_substring("", true) was essentially not checking anything, resetting the err stream, and then moving on.