qt: test: Create at most one testing setup

MarcoFalke commented at 8:36 pm on June 26, 2019: member

It is assumed that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).

This assumption is violated in the GUI tests, as a testing setup is created as the first step of the main function and then (sometimes) another one for the following test cases.

So, the gui tests create two testing setups:

BasicTestingSetup in main (added in fa4a04a5a942d582c62773d815c7e1e9897975d0)
a testing setup for individual test cases

Avoid that by destructing the testing setup in main after creation and then move the explicit ECC_Stop to the only places where it is needed (before and after apptests).

MarcoFalke force-pushed on Jun 26, 2019

DrahtBot added the label GUI on Jun 26, 2019

DrahtBot added the label Tests on Jun 26, 2019

DrahtBot commented at 9:09 pm on June 26, 2019: member

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

Conflicts

No conflicts as of last run.

Sjors commented at 9:24 pm on June 26, 2019: member

fa3b441 works for me on macOS 10.14.5, but Travis looks quite unhappy.

MarcoFalke force-pushed on Jun 26, 2019

DrahtBot added the label Needs rebase on Jun 26, 2019

MarcoFalke closed this on Jun 26, 2019

MarcoFalke deleted the branch on Jun 26, 2019

ryanofsky commented at 4:04 pm on June 27, 2019: member

Why’s this closed? It’s confusing when a PR or issue is closed with no comment.

MarcoFalke restored the branch on Jun 27, 2019

MarcoFalke commented at 4:40 pm on June 27, 2019: member

I didn’t think it was worth it, but let’s see what others think.

MarcoFalke reopened this on Jun 27, 2019

MarcoFalke force-pushed on Jun 27, 2019

DrahtBot removed the label Needs rebase on Jun 27, 2019

in src/qt/test/rpcnestedtests.cpp:37 in fa1a27231c outdated

33@@ -34,9 +34,6 @@ void RPCNestedTests::rpcNestedTests()
34     tableRPC.appendCommand("rpcNestedTest", &vRPCCommands[0]);
35     //mempool.setSanityCheck(1.0);
36 
37-    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference

ryanofsky commented at 5:22 pm on June 27, 2019:

In commit “qt: test: Create at most one testing setup” (fac5bb8f2e135090cc21bdde80ff2adc4c1a1401)

It seems better to keep these lines here than to move them to main with the comment “started by the AppTests later on”. Why move code that is specific to apptests outside of apptests?

Similarly it seems like the BasicTestingSetup object in main could just be an apptests member if it’s going to be destroyed as soon as apptests finish.

MarcoFalke commented at 6:33 pm on June 27, 2019:

Fixed

ryanofsky approved

ryanofsky commented at 5:25 pm on June 27, 2019: member

utACK fac5bb8f2e135090cc21bdde80ff2adc4c1a1401

Change seems fine, but I’m also confused about why the issue #16288 was closed (also without comment). Does this PR not fix that issue? Or is a different fix for the issue better? Or is the issue not valid?

MarcoFalke commented at 5:53 pm on June 27, 2019: member

The comment was by github, it said:

MarcoFalke closed this in #16289 19 hours ago

So the issue itself was already fixed with the linked pull request.

MarcoFalke added the label Refactoring on Jun 27, 2019

Sjors commented at 6:06 pm on June 27, 2019: member

#16289 was the easiest way to fix a broken master branch for macOS tests, but this PR seems like a more thorough long term solution. I don’t entirely understand the original problem though.

ryanofsky commented at 6:16 pm on June 27, 2019: member

Thanks, I didn’t understand the github comment. I also don’t fully understand the original issue, or why BasicTestingSetup can’t seem to clean up after itself, and seems to need additional ECC_Stop() / DisconnectTestLogger() calls.

MarcoFalke force-pushed on Jun 27, 2019

MarcoFalke commented at 6:36 pm on June 27, 2019: member

I also don’t fully understand the original issue, or why BasicTestingSetup can’t seem to clean up after itself

It does clean up after itself, but the assumption is that ideally only one BasicTestingSetup exists at any point in time for each process (due to use of globals).

We violate this assumption in the GUI tests, as a testing setup is created as the first step of the main function and then (sometimes) another one for the following test cases.

in src/qt/test/apptests.cpp:86 in 999999bb11 outdated

82@@ -82,6 +83,8 @@ void AppTests::appTests()
83     // Reset global state to avoid interfering with later tests.
84     AbortShutdown();
85     UnloadBlockIndex();
86+    ECC_Stop();

ryanofsky commented at 6:51 pm on June 27, 2019:

In commit “qt: test: Create at most one testing setup” (999999bb11de41136b165e679b5828c4786a2f0f)

Why are ECC_Stop and DisconnectTestLogger needed here? Aren’t they about to be called in the BasicTestingSetup destructor on the next line? It might be helpful to say something in a comment here, depending on the reason.

MarcoFalke commented at 8:51 pm on June 27, 2019:

Removed

ryanofsky approved

ryanofsky commented at 6:55 pm on June 27, 2019: member

utACK 999999bb11de41136b165e679b5828c4786a2f0f. It definitely seems good to have only have one BasicTestingSetup active at a time.

qt: test: Create at most one testing setup faa1e0fb17

MarcoFalke force-pushed on Jun 27, 2019

laanwj commented at 11:44 am on July 3, 2019: member

It definitely seems good to have only have one BasicTestingSetup active at a time.

would it make sense to enforce this with an assertion? we intentionally not trying to support this case, so crashing the tests with an assertion seems preferable to the unexpected problems that might happen otherwise

MarcoFalke commented at 11:47 am on July 3, 2019: member

There sort of already is an assertion in ECC_Start(), which can not be called twice, unless there is a ECC_Stop() in between. The same is true for opening the debug log twice.

This was missed in the GUI tests because it was intentionally overwritten (by calling ECC_Stop and closing the debug log file)

laanwj commented at 11:49 am on July 3, 2019: member

This was missed in the GUI tests because it was intentionally overwritten (by calling ECC_Stop and closing the debug log file)

Whoops.

code review ACK faa1e0fb1712b1f94334e42794163f79988270fd

laanwj merged this on Jul 3, 2019

laanwj closed this on Jul 3, 2019

laanwj referenced this in commit 38fbb575e2 on Jul 3, 2019

MarcoFalke deleted the branch on Jul 3, 2019

sidhujag referenced this in commit f3ddc4d541 on Jul 5, 2019

deadalnix referenced this in commit 78c6e0e1a1 on Mar 26, 2020

Munkybooty referenced this in commit 4afdbfa9d3 on Nov 4, 2021

vijaydasmp referenced this in commit 50fe41ef6d on Dec 6, 2021

vijaydasmp referenced this in commit 8f63dc55cf on Dec 10, 2021

vijaydasmp referenced this in commit c6beb43904 on Dec 13, 2021

vijaydasmp referenced this in commit e8e967c9a1 on Dec 13, 2021

vijaydasmp referenced this in commit f659d98cc6 on Dec 15, 2021

DrahtBot locked this on Dec 16, 2021

qt: test: Create at most one testing setup #16294

Conflicts