[WIP] Unit test sub-directories #12583

pull chrislennon wants to merge 1 commits into bitcoin:master from chrislennon:unittest_subdir changing 5 files +24 −9

chrislennon commented at 8:47 PM on March 2, 2018: none
fixes #12574

First time contributing - could do with some advice completing this.

To me, issue consists of three parts:
- Creation of sub-directory within temp work-space
  
  I have low confidence that I approached this correctly (though I believe it works)
- Appending the name of the test into the folders the framework is created
  
  TODO - would appreciate a nudge in the correct direction to picking out the name of the executing test
- Iteration number of the test
  
  TODO - Relating to the 2nd point, my logic would be when I have the name of the test, check the file system and iterate the trailing int i.e. dbwrapper_1 to dbwrapper_2
I'm happy to iterate this PR a few times until its acceptable, figured it was more sensible to seek advice than declare failure and bin the code 😉
adds function for creation of sub-directory within temp workspace 7904ed8287
fanquake added the label Tests on Mar 3, 2018
laanwj commented at 2:43 PM on March 5, 2018: member

Concept ACK - thanks for addressing this.

in src/fs.cpp:15 in 7904ed8287

  11 | @@ -12,4 +12,18 @@ FILE *freopen(const fs::path& p, const char *mode, FILE *stream)
  12 |      return ::freopen(p.string().c_str(), mode, stream);
  13 |  }
  14 |  
  15 | +// Create and return temporary subdirectory for test outputs

laanwj commented at 2:44 PM on March 5, 2018:

Small nit: as this is specific to the tests, I'd prefer moving this function to a unit-test specific utilities file, creating one if necessary -e.g. src/test/test_utils.cpp.

laanwj commented at 2:46 PM on March 5, 2018: member

would appreciate a nudge in the correct direction to picking out the name of the executing test

We could do all kind of metadata introspection. Maybe boost has a way to do that. Or we could just pass the subdirectory name to the function unit_test_directory() - easy and straightforward.

Iteration number of the test

Same as above, the straightforward solution would be to (optionally) pass it in.
ccdle12 commented at 9:38 AM on March 12, 2018: contributor

Hi guys,

I'm a new contributor, was looking into this PR and was just wondering if I was re-compiling the tests correctly?

For Example, I've commented out all the tests in dbwrapper_tests.cpp and ran make to compile the tests.

Now when I run test_bitcoin --log_level=all --run_test=dbwrapper_tests, I can see all the tests in dbrwapper_tests are being run even though I've commented out all the tests and re-compiled?

Apologies if it's a silly question, I just wanted to see a "cause and effect" before addressing the issue of organising the tests into sub folders in the temp directory.
chrislennon commented at 9:59 AM on March 12, 2018: none

@ccdle12 I haven't experienced this directly (nor tested your above approach), typically I'm rebuilding the tests (make -C src/test) and running them all (./test_bitcoin) and I see the effect of my changes.

If you are looking to pick this up, (whilst not complete) I made some small progress on the comments above on my PR-fixes branch over on my fork which may assist.

Currently it covers the comment from @laanwj about changing the location of the function to a more appropriate location and the names of the unit tests. The last part it doesn't yet cover is the iteration number of the named folders.
ccdle12 commented at 10:02 AM on March 12, 2018: contributor

@chrislennon Thanks, I'll try your approach to rebuilding the tests and also look into the PR changes
in src/fs.cpp:24 in 7904ed8287
```
  19 | +
  20 | +    if(fs::create_directory(dir) || fs::exists(dir))
  21 | +    {
  22 | +        return dir;
  23 | +    }
  24 | +    else{
```
MarcoFalke commented at 2:49 PM on March 12, 2018:

Don't forget to install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

in src/test/test_bitcoin.cpp:71 in 7904ed8287

  67 | @@ -68,7 +68,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
  68 |  
  69 |          RegisterAllCoreRPCCommands(tableRPC);
  70 |          ClearDatadirCache();
  71 | -        pathTemp = fs::temp_directory_path() / strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000)));
  72 | +        fs::path pathTemp = fsbridge::unit_test_directory() / strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(100000)));

MarcoFalke commented at 2:55 PM on March 12, 2018:

Why would you shadow the member? Also, why wouldn't you need to touch the gui tests?

MarcoFalke added the label Up for grabs on Mar 22, 2018
fanquake closed this on Apr 26, 2018
MarcoFalke removed the label Up for grabs on Apr 26, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12574 Unit tests: create one top-level temporary directory #13067 [WIP] Unit test sub-directories - Continued