Skip to content

Changed Test Clause to CHECK()#1215

Closed
ShikharJ wants to merge 2 commits intosymengine:masterfrom
ShikharJ:ImproveTests
Closed

Changed Test Clause to CHECK()#1215
ShikharJ wants to merge 2 commits intosymengine:masterfrom
ShikharJ:ImproveTests

Conversation

@ShikharJ
Copy link
Copy Markdown
Member

This is a standing PR for now. It can be updated whenever required (in the future).

r3 = add(add(add(r1, r2), integer(1)), real_double(0.2));
REQUIRE(is_a<RealDouble>(*r3));
REQUIRE(std::abs(down_cast<const RealDouble &>(*r3).i - 1.8) < 1e-12);
CHECK(is_a<RealDouble>(*r3));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not correct. If this check fails the check below will crash. This has to be a REQUIRE

@isuruf
Copy link
Copy Markdown
Member

isuruf commented Feb 21, 2017

I don't know whether this is really necessary, changing 45 files and creating conflicts with all the other PRs.

@ShikharJ
Copy link
Copy Markdown
Member Author

That's the reason why I mentioned that it is intended as a standing PR, for whenever the need may be (in the future). There had been a number of instances with me, while implementing stuff, where the tests failed but running ctest --output-on-failure only returned the name of the test block, and I had to individually check for each failure (which could be anything from a silly typo to an implementation error).

@ShikharJ ShikharJ force-pushed the ImproveTests branch 2 times, most recently from c162976 to 1e2df9d Compare April 3, 2017 16:14
@rikardn
Copy link
Copy Markdown
Contributor

rikardn commented Apr 14, 2021

Although this might be a good idea I think due to its many conflicts it will not be worth to resurrect this PR. Can we do some spring cleaning and close?

@ShikharJ
Copy link
Copy Markdown
Member Author

@rikardn I think we should just close. The codebase has changed significantly since, and there isn't a point in resurrecting this, since the potential advantages aren't much over the disadvantages.

@ShikharJ ShikharJ closed this Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants