made TestEnv able to run the tests in parallel, which reduced the tes…#146
made TestEnv able to run the tests in parallel, which reduced the tes…#146ChrisSon15 merged 3 commits intobisq-network:mainfrom
Conversation
…t time on my end from 4m to 1m. Also remove unneeded tmp_dir
stejbac
left a comment
stejbac
left a comment
There was a problem hiding this comment.
In addition to the in-code suggestions, we should remove simple-semaphore from the Cargo manifest, as it is no longer a crate dependency after these changes.
| // let permit = SEMAPHORE.acquire(); // have testenvs single threaded because of bitcoind | ||
| // and electrs references. | ||
| let tmp_dir = tempdir().expect("failed to create temporary directory"); | ||
| std::env::set_current_dir(tmp_dir.path()).expect("failed to set current directory"); |
There was a problem hiding this comment.
I don't see how setting the current directory can possibly work in parallel, without being protected by a global mutex/semaphore held by whatever is relying on it being a particular path.
I really don't think anything (other than possibly some very top level application startup code) should care about the current directory, and it should be completely unaffected by its value. If that's not the case, we should change it.
--
Furthermore, after removing the _tmp_dir field from the struct, the tmp_dir local variable will be immediately dropped after having set the current directory to it, so the current directory will be pointing to an invalid/stale path.
There was a problem hiding this comment.
The tmp_dir was a left over not used.
Temp directories now need to be created when needed.
thanks for spotting
sdmg15
left a comment
sdmg15
left a comment
There was a problem hiding this comment.
Tests are flaky. This is related to the comment made by stejbac.
stejbac
left a comment
stejbac
left a comment
There was a problem hiding this comment.
utACK
(simple-semaphore still needs to be removed from the testenv Cargo manifest, though.)
There was a problem hiding this comment.
Not sure you really want to push this?
There was a problem hiding this comment.
yes, but will delete it soon when no longer needed. So its just temporarly
3 participants
…t time on my end from 4m to 1m.
Also remove unneeded tmp_dir