scripted-diff: Sort test includes #18673

1. 
   MarcoFalke commented at 5:15 pm on April 16, 2020: member

   When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have clang-format installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.

   This pull preempts both issues by just sorting all includes in one commit.

   Please be aware that this is NOT a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.

   Edit: Also includes a commit to bump the copyright headers, so that the touched files don’t need to be touched again for that.

2. MarcoFalke added the label Refactoring on Apr 16, 2020
3. MarcoFalke added the label Tests on Apr 16, 2020
4. scripted-diff: Sort test includes

   -BEGIN VERIFY SCRIPT-
    # Mark all lines with #includes
    sed -i --regexp-extended -e 's/(#include <.*>)/\1 /g' $(git grep -l '#include' ./src/bench/ ./src/test ./src/wallet/test/)
    # Sort all marked lines
    git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
   -END VERIFY SCRIPT-

   fac5c37300
5. scripted-diff: Bump copyright headers

   -BEGIN VERIFY SCRIPT-
   ./contrib/devtools/copyright_header.py update ./
   -END VERIFY SCRIPT-

   fa488f131f
6. MarcoFalke force-pushed on Apr 16, 2020
7. 
   DrahtBot commented at 7:57 pm on April 16, 2020: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #18575 (bench: Remove requirement that all benches use same testing setup by MarcoFalke)
   • #18432 (util: Make our stringstream usage locale independent by practicalswift)
   • #18242 (Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli)
   • #18000 (Index for UTXO Set Statistics by fjahr)
   • #16127 (More thread safety annotation coverage by ajtowns)
   • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

8. 
   promag commented at 11:45 pm on April 16, 2020: member

   ACK just for the scripted-diff 👏

   will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort

   or not stage in the first place?

9. 
   jnewbery commented at 2:15 am on April 17, 2020: member

   I think generally accepted good practice is to go from local to global for includes (ie file’s header, then local project imports, then library imports, then std imports). See https://stackoverflow.com/questions/2762568/c-c-include-header-file-order. That seems to be the de facto standard across much of this repo.

   Is there any way you can modify your scripted diff to preserve this ordering?

10. test: Move boost/stdlib includes last fa4632c417
11. 
    MarcoFalke commented at 10:44 am on April 17, 2020: member

    @jnewbery I went ahead and manually fixed up the 7 files where the boost/stdlib includes were not last. Those files had this issue as a pre-existing condition and this is not something that can be solved by clang-format. The original author of the test should have take care of it. I am not going to touch files that are not already touched by this pull request and if you think this is a worthy task, it can be done as a follow-up cleanup for the other files. I am happy to review and ACK, but I think the goal of this pull should not be extended further than solving a workflow issue of devs writing tests and using clang-format.
12. in src/test/util_tests.cpp:9 in fa4632c417

     5@@ -6,18 +6,18 @@
     6 
     7 #include <clientversion.h>
     8 #include <hash.h> // For Hash()
     9-#include <key.h> // For CKey
    10+#include <key.h>  // For CKey
    

    ---

    ---

    

    jonatack commented at 12:10 pm on April 17, 2020:

    Was this changed manually?

    ---

    

    MarcoFalke commented at 1:06 pm on April 17, 2020:

    No, it is part of the scripted-diff, which you can verify by running ./test/lint/commit-script-check

    ---

    

    jonatack commented at 1:17 pm on April 17, 2020:

    Thanks – I need to upgrade my scripted-diff-fu
13. 
    jonatack commented at 12:30 pm on April 17, 2020: member

    ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
14. 
    practicalswift commented at 12:56 pm on April 17, 2020: contributor

    ACK fa4632c41714dfaa699bacc6a947d72668a4deef

    A scripted-diff tour de force - I’m impressed. Very clever! :)

15. 
    jnewbery commented at 2:13 pm on April 17, 2020: member

    Very rough code review ACK. Thanks Marco!
16. MarcoFalke merged this on Apr 17, 2020
17. MarcoFalke closed this on Apr 17, 2020

    ---

18. MarcoFalke deleted the branch on Apr 17, 2020
19. DrahtBot locked this on Feb 15, 2022

Contributors
MarcoFalke DrahtBot promag jnewbery jonatack practicalswift

Labels
Refactoring Tests