- User Since
- May 14 2017, 13:52 (135 w, 18 h)
Fri, Dec 13
That is not a good solution if you need to specify the parallelism level for the test. ninja already does job scheduling.
Add a test.
Wed, Dec 11
It overall looks, good, expect the fact that many nits should have been done prior to the back port, and not have a handful of them in.
Is there a reason why the original patch is backported as this, even though it is known to be bogus, and then this?
If you are going to clean this up, actually clean it up. There are other dead functions, in fact there is a function that is declared and NEVER defined anywhere.
Could you do the same for SCRIPT_ENABLE_SCHNORR_MULTISIG and SCRIPT_VERIFY_CHECKDATASIG_SIGOPS, please?
Some small changes, but LGTM
One small change, but otherwise, LGTM
Tue, Dec 10
Then the test doesn't test what it is supposed to be testing.
That looks better, let me do a pass over this tomorow.
Address comments, except making the constructor of the test map element explicit, as it makes the test way more complex and the upside is not very clear.
Mon, Dec 9
PR16161 was merged.
Sun, Dec 8
Adding tests would be good.
Fri, Dec 6
Back on your queue. It looks like this broke the rpc_bind test.
It would be preferable to remove the tests along side the code that they do test. Doing this as this simply reduce test coverage.
Thu, Dec 5
Using goto to not litter otherwise good code with error handling is a fairly common practice. Surely, if this makes this code bad in some way, the specifics can be pointed at rather than blanket statement about goto being harmful.
Remove now unecessary matchKey
Wed, Dec 4
While this is a pretty bad idea to have said config, it is an even worse idea to have a unit test depends on user config, not testing the software, yet ran as part as its test suite by default, running a buggy script.
As it turns out, having test that depends on the user's global config, not testing the actual software but running by default on the software test suite.
Looks like you broke the build.
Please mention PR1243653 (with no # in the middle) in the description so that it can be searched for.
That whole thing depends on a gazillion environments variables, internet connectivity, and whatever else, and doesn't check at all that bitcoin-abc is working. I really don't see why someone who want to checkout the ABC sources and run the tests to make sure it works have to go through the bullshit of installing arc, setting up an auth token and alike. That makes no sense whatsoever. The software is supposed to get better *FOR USERS* over time. What does it do in the middle of ninja check? How come, when the output of ninja check is polluted, nobody stops one second to ask themsleves "Wait, why is that thing in the output to begin with? Does it need to be?" but instead go on changing all the output formatting so that the main problem remains, but is less visible?
Do not deprecate BIP70.
Tue, Dec 3
You probably want to investigate why the build failed.
Mon, Dec 2
The general direction is good, but there are a few problem with this patch.