Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Protect for table lookup zero divisor #10465

Merged
merged 5 commits into from
Apr 12, 2024
Merged

Protect for table lookup zero divisor #10465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
83a064b
Added unit test for zero normalization value, fails right now
Myoldmopar Apr 12, 2024
08840ed
Added fatal error trap for zero normalization divisor, unit test stil…
Myoldmopar Apr 12, 2024
a0e35ba
Fixes up unit test to catch error message during curve get input
Myoldmopar Apr 12, 2024
4f568fb
Modified unit test to use a new worker function for err comparison
Myoldmopar Apr 12, 2024
4188a59
Use std::abs instead of fabs, thanks @jmarrec
Myoldmopar Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/EnergyPlus/CurveManager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2494,6 +2494,11 @@ namespace Curve {

if (normalizeMethod != NM_NONE && fields.count("normalization_divisor")) {
normalizationDivisor = fields.at("normalization_divisor").get<Real64>();
if (std::abs(normalizationDivisor) < std::numeric_limits<Real64>::min()) {
ShowSevereError(
state, format("Table:Lookup named \"{}\": Normalization divisor entered as zero, which is invalid", thisCurve->Name));
ErrorsFound = true;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if the value coming in is zero (or essentially zero). This is only hit if the normalization divisor is going to be used (based on other input fields). I'm open to wording improvement, but it should get the job done.

}

std::vector<double> lookupValues;
Expand Down
42 changes: 42 additions & 0 deletions tst/EnergyPlus/unit/CurveManager.unit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,48 @@ TEST_F(EnergyPlusFixture, DivisorNormalizationDivisorOnly)
}
}

TEST_F(EnergyPlusFixture, DivisorNormalizationDivisorOnlyButItIsZero)
{
std::string const idf_objects = delimited_string({"Table:Lookup,",
"y_values, !- Name",
"y_values_list, !- Independent Variable List Name",
"DivisorOnly, !- Normalization Method",
"0.0, !- Normalization Divisor",
"2.0, !- Minimum Output",
"21.0, !- Maximum Output",
"Dimensionless, !- Output Unit Type",
", !- External File Name",
", !- External File Column Number",
", !- External File Starting Row Number",
"2.0, !- Value 1",
"4.0; !- Value 2",

"Table:IndependentVariableList,",
"y_values_list, !- Name",
"x_values_1; !- Independent Variable Name 1",

"Table:IndependentVariable,",
"x_values_1, !- Name",
", !- Interpolation Method",
", !- Extrapolation Method",
", !- Minimum value",
", !- Maximum value",
", !- Normalization Reference Value",
"Dimensionless !- Output Unit Type",
", !- External File Name",
", !- External File Column Number",
", !- External File Starting Row Number",
",",
"2.0, !- Value 1",
"3.0; !- Value 3"});

ASSERT_TRUE(process_idf(idf_objects));
EXPECT_EQ(0, state->dataCurveManager->NumCurves);

EXPECT_THROW(Curve::GetCurveInput(*state), std::runtime_error);
EXPECT_TRUE(this->compare_err_stream_substring("Normalization divisor entered as zero"));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New unit test that sets up a simple table lookup with zero valued normalization divisor. It first makes sure the runtime error is thrown during curve get input, and then also makes sure there is a specific mention of the normalization divisor in the error output. Note I'm not comparing the whole error output because I don't care about overall output message structure, I just want to make sure the divisor error is, in fact, emitted.


TEST_F(EnergyPlusFixture, DivisorNormalizationAutomaticWithDivisor)
{
// /*
Expand Down
9 changes: 9 additions & 0 deletions tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,15 @@ bool EnergyPlusFixture::compare_err_stream(std::string const &expected_string, b
return are_equal;
}

bool EnergyPlusFixture::compare_err_stream_substring(std::string const &search_string, bool reset_stream)
{
auto const stream_str = this->err_stream->str();
bool const found = stream_str.find("Normalization divisor entered as zero") != std::string::npos;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant search_string instead of harcoding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I wanted it to search for this hard coded string.

.

.

.
...

OK, OK, I'll slow down. Thank you an awful lot, @jmarrec ! Fix incoming soon - tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading only the first bit on my phone in the email notification cracked me up ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang-tidy 15 definitely catches it for me. I wonder why the cppcheck didn't? And that's because we only run cppcheck in ./src :)

EXPECT_TRUE(found);
if (reset_stream) this->err_stream->str(std::string());
return found;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new worker function on the unit test fixture. This allows you to compare some specific search phrase in the ERR contents without having to list the entire formatted ERR message. See the unit test above in this PR for an example.


bool EnergyPlusFixture::compare_cout_stream(std::string const &expected_string, bool reset_stream)
{
auto const stream_str = this->m_cout_buffer->str();
Expand Down
9 changes: 8 additions & 1 deletion tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,13 @@ protected:
// Will return true if string matches the stream and false if it does not
bool compare_err_stream(std::string const &expected_string, bool reset_stream = true);

// Check if ERR stream contains a substring. The default is to reset the ERR stream after every call.
// It is easier to test successive functions if the ERR stream is 'empty' before the next call.
// This calls EXPECT_* within the function as well as returns a boolean so you can call [ASSERT/EXPECT]_[TRUE/FALSE] depending
// if it makes sense for the unit test to continue after returning from function.
// Will return true if string is found in the stream and false if it is not
bool compare_err_stream_substring(std::string const &search_string, bool reset_stream = true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New worker function in fixture header so it can be used by unit tests.


// Compare an expected string against the COUT stream. The default is to reset the COUT stream after every call.
// It is easier to test successive functions if the COUT stream is 'empty' before the next call.
// This calls EXPECT_* within the function as well as returns a boolean so you can call [ASSERT/EXPECT]_[TRUE/FALSE] depending
Expand Down Expand Up @@ -292,7 +299,7 @@ private:
// This function should be called by process_idf() so unit tests can take advantage of caching
// To test this function use InputProcessorFixture
// This calls EXPECT_* within the function as well as returns a boolean so you can call [ASSERT/EXPECT]_[TRUE/FALSE] depending
// if it makes sense for the unit test to continue after retrning from function.
// if it makes sense for the unit test to continue after returning from function.
// Will return false if no errors found and true if errors found

// static bool process_idd(std::string const &idd, bool &errors_found);
Expand Down
Loading