Switch testing framework from MAIN to new UNITTEST network (rebase) #5030

1. 
   laanwj commented at 9:25 am on October 2, 2014: member

   See #4845. Trying to figure out why this breaks the tests now.
2. 
   laanwj commented at 11:45 am on October 2, 2014: member

   This has to do with the order in which tests are executed.

   blockv2_tests.cpp leaves the block chain in a unspecified state, which causes the miner tests to fail.

   I’m going to split this up into two commits: one that introduces the UNITTEST framework and modifyiable parameters, and one that adds the block v2 test.

3. Switch testing framework from MAIN to new UNITTEST network

   UNITTEST inherites from MAIN but allows synamically changing its parameters using the ModifiableParams() interface

   f0fd00cb77
4. Avoid introducing a virtual into CChainParams

   Treat fSkipProofOfWorkCheck the same as other parameters.

   fbd36d8fb5
5. laanwj force-pushed on Oct 2, 2014
6. 
   laanwj commented at 11:55 am on October 2, 2014: member

   This should pass now.
7. 
   laanwj commented at 11:57 am on October 2, 2014: member

   @SergioDemianLerner I’ve split off this part that adds the blockv2 tests: https://github.com/laanwj/bitcoin/commit/782f6c8758ea25dcc93989e8631ba4f34b5ae09e It needs some more work to leave the block chain in a usable state for the next test, or to create a new block chain for each test. This already hinted in the comments

   0// We don't know the state of the block-chain here: it depends on which other tests are run before this test.
   1// See [#4688](/bitcoin-bitcoin/4688/) for a patch that allows the re-creation of the block-chain
   2// for each testcase that requires it.
   

8. laanwj renamed this:
   Test rebased #4845
   Switch testing framework from MAIN to new UNITTEST network (rebase)
   on Oct 2, 2014
9. laanwj merged this on Oct 2, 2014
10. laanwj closed this on Oct 2, 2014

    ---

11. laanwj referenced this in commit 45c41c05a0 on Oct 2, 2014
12. 
    SergioDemianLerner commented at 2:08 pm on October 2, 2014: contributor

    The problem is not that the blockv2_test leaves the blockchain in a bad state. The problem is the the miner_test MUST be executed before blockv2_test because miner_tests tests v1 blocks and blockv2_test tests v2 blocks. If the Boost test framework does not allow to specify a unique order for the tests, one way to force this is to include blockv2_test into miner_test via #include, and let both of them be contained in a single .cpp file
13. 
    SergioDemianLerner commented at 2:14 pm on October 2, 2014: contributor

    Definitively, the easiest way would be that each new test that mines blocks adds an #include line into miner_test.cpp @laanwj : I suppose I don’t have write access to the rebased repo. Could you add the #include “blockv2_test.cpp” into the last line of miner_test.cpp and create a new pull request for that?
14. 
    laanwj commented at 2:59 pm on October 2, 2014: member

    I don’t really like tests that are dependent on order, or hacking around that; let’s try to do it properly. What we really need is #4688.

    I can’t give you write access to my branch, but if you want to edit the branch you could pull my branch and start from there.

15. 
    SergioDemianLerner commented at 3:35 pm on October 2, 2014: contributor

    Yes I agree, but I’m also more pragmatic and not so purist: why not allowing both? Some future tests could be dependent on the order and some not. The overhead of recreating the block-chain is low, but we could avoid it in some cases. Other reason to let people add block test cases to a single file is that test cases involving transactions require pre-mined coinbases. If each test must recreate the block-chain, it must start with mining 100 blocks in order to have 50 BTC mature.

    As an example, most of the block tests in TheBlueMatt FullBlockTestGenerator.java are independent of order. (off-topic: nevertheless they are painfully linked together with references to previous blocks: Block b47 = createNextBlock(b46, ….), this can be avoided by an internal reference of the last tip)

    So we could create a single empty file “block_tests.cpp” where people can easily add tests by including their own files using #include at the end, and at the same time, for the tests that require a new block-chain, people put them in separate files, and they clear the block-chain before each test using #4688

16. 
    SergioDemianLerner commented at 3:39 pm on October 2, 2014: contributor

    If somebody were to re-write each test case of FullBlockTestGenerator.java as an independent .cpp file, the block-chain would need to be re-created 40 times in each test_bitcoin execution.
17. 
    laanwj commented at 7:07 pm on October 2, 2014: member

    If some starting point is convenient to be used by multiple tests (like “100 blocks in order to have 50 BTC mature”) it makes sense to compute that once and cache it.

    I still don’t see a good reason to make tests use each other’s output. Then it becomes impossible to disable tests without affecting others. I think we’ve all worked with hairballs like that, so I’d prefer to avoid it. They are called ‘unit tests’ for a good reason.

    The whole reason for merging this (and allowing the skip proof-of-work) was to make building blockchains extremely cheap, so unless the overhead becomes serious I’d prefer to just start from scratch for each test. But if necessary I’d prefer caching/precomputation to letting one test use the output of another.

18. 
    sipa commented at 7:30 pm on October 2, 2014: member

    @SergioDemianLerner Please don’t confuse the unit tests (in .cpp code) and the network interaction tests (in comparison tool). The purpose of the first is to test whether the code behaves as the implementer intends it to work. The purpose of the second is to detect consensus and other cross-implementation compatibility.

    I would be strongly opposed to converting comparison tool into a unit test. It means that for example a change in serialization code keeps bitcoind compatible with itself, but not with other nodes on the network (including older versions of bitcoind).

    I am in favor of replacing comparison tool with a more general framework (using no Bitcoin core code), preferably in a different language, that replays scenarios of network interactions, and verifies that state running bitcoind (or other code) behaves as expected. See #4545.

19. 
    SergioDemianLerner commented at 1:36 pm on October 3, 2014: contributor

    I don’t say we should replace the comparison tool, since it has been very useful. It was an example of how the number of tests could grow. But some unit test can test things that are hidden from the remote nodes. I made bitcoind stress unit tests that check RAM and CPU usage, and I’m waiting to merge them when we resolve how.
20. 
    sipa commented at 3:23 pm on October 3, 2014: member

    Ok, good. I misinterpreted your comment it seems.
21. 
    SergioDemianLerner commented at 12:19 pm on October 6, 2014: contributor

    This has been merged! I hadn’t notice. Great! Thanks!
22. 
    mikehearn commented at 10:34 am on October 8, 2014: contributor

    In bitcoinj we have a large number of unit tests that create block chains and throw them away. Actually a full test run of bitcoinj runs ~5000 unit tests, although not all of those require block chains. So there’s no reason to be scared of creating and throwing away chains - they are after all just in memory data structures and a UNITTEST params chain can have blocks mined more or less instantly.

    The problem here is not performance or how to split things up, it’s that Satoshi didn’t write testable code in the first place. Lots of global variables everywhere instead of objects. Not much we can do about that except refactor it into a testable state very, very carefully.

23. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj SergioDemianLerner sipa mikehearn