Fix ScriptNum Tests #22786

pull fpelliccioni wants to merge 1 commits into bitcoin:master from fpelliccioni:fix_scriptnum_test changing 1 files +19 −19

fpelliccioni commented at 12:15 AM on August 24, 2021: none
Summary

At first glance the test scriptnum_tests/operators has some issues:
- It is not testing what it really want to test (I think).
- If size(offsets) > size(values) the test program will crash.
I also took the opportunity to modernize the scriptnum_tests/creation code, but the semantics of the new code should be equivalent to that of the previous code.

Test Plan
1. Build
2. ./src/test/test_bitcoin -t scriptnum_tests/operators
3. ./src/test/test_bitcoin -t scriptnum_tests/creation
fix tests ea5fdbf464
fpelliccioni renamed this:
~~fix tests~~
Fix ScriptNum Tests
on Aug 24, 2021
DrahtBot added the label Tests on Aug 24, 2021
in src/test/scriptnum_tests.cpp:188 in ea5fdbf464
```
 202 | -            RunOperators(values[i] - values[j], values[i] - values[j]);
 203 | +            RunOperators(a, a);
 204 | +            RunOperators(a, -a);
 205 | +            RunOperators(a, b);
 206 | +            RunOperators(a, -b);
 207 | +            RunOperators(a + b, b);
```
meshcollider commented at 12:26 PM on August 24, 2021:

I suspect these should have been of the form value + offset rather than value + value

cculianu commented at 2:16 PM on August 24, 2021:

Hmm. Good question. The original PR is #3965 @theuni (original PR author) -- What was the intention here?
rajarshimaitra commented at 6:15 PM on October 7, 2021: contributor

Concept ACK on code change. Seems cleaner.

But I am confused. This doesn't seem to address the issues mentioned in the PR description. Only the modernization.

Can you explain what did you mean by "it's not checking what it intends to check"?
laanwj commented at 2:20 PM on November 10, 2021: member

I'm also a bit confused here. What is the intent of the original code, what changed here. Next time, please separate out "modernization" from fixes into separate commits at least (it's fine to have them in the same PR).
laanwj commented at 3:47 PM on December 17, 2021: member

Closing this for now. Please let me know if you intend to continue work on this and address the comments.
laanwj closed this on Dec 17, 2021
DrahtBot locked this on Dec 17, 2022

Contributors

Labels

Fix ScriptNum Tests #22786

Summary

Test Plan