Conversation
|
Thanks for the PR. Can you fix the merge conflicts? |
|
Updated @isuruf. |
symengine/tests/basic/test_basic.cpp
Outdated
| CHECK(i2->compare(*i2) == 0); | ||
| CHECK(i2->compare(*i3) == -1); | ||
| CHECK(i3->compare(*i2) == 1); | ||
| REQUIRE(x->compare(*x) == 0); |
There was a problem hiding this comment.
Why this change? CHECK is actually better than REQUIRE, because even if a test fail, the rest of the tests will be run.
There was a problem hiding this comment.
So can the rest of the cases be changed to CHECK?
There was a problem hiding this comment.
It should, but that's a big change and I would only change it for the future
There was a problem hiding this comment.
Ok. I'd like to take that up whenever it's relevant.
symengine/tests/basic/test_basic.cpp
Outdated
|
|
||
| c1 = Complex::from_two_nums(*integer(2), *integer(5)); | ||
| c2 = real_double(0.8); | ||
| CHECK_THROWS_AS(c1->rsub(*c2), NotImplementedError); |
There was a problem hiding this comment.
Don't call rsub directly. Use c1->sub(*c2).
subnum(c1, c2) is even better.
There was a problem hiding this comment.
I must have accidentally made that. I'll change it.
| SYMENGINE_C_ASSERT(!is_a_Integer(e)); | ||
|
|
||
| rational_set_si(e, 100, 47); | ||
| s = basic_str(e); |
There was a problem hiding this comment.
Value of s needs to be freed before assigning.
| CHECK(is_a<Integer>(*r1)); | ||
| CHECK(r1->__eq__(*integer(3))); | ||
| REQUIRE(is_a<Integer>(*r1)); | ||
| REQUIRE(r1->__eq__(*integer(3))); |
There was a problem hiding this comment.
Can you change this back to the original?
isuruf
left a comment
isuruf
left a comment
There was a problem hiding this comment.
Thanks a lot for this PR. +2% coverage is a lot.
|
Stay tuned. More is to come :) |
|
@ShikarhJ, can you fix the merge conflicts? |
|
Updated @isuruf |
Misc Test Improvements
2 participants
Please review and suggest improvements.