From b82b3f40673f2d3c8ec73c075b645890480b74b7 Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Tue, 18 Jul 2017 11:57:02 +0200 Subject: [PATCH] Fix the skip-check in TestMethods::invokeTest() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestMethods::invokeTest() has an outer loop on global data (albeit with a comment that said otherwise). On its first cycle, we run the test function's *_data() method, if it has one; there is an inner loop on the rows this created. If the *_data() QSKIP()s, we need to skip the whole test; otherwise, a QSKIP() in one sub-test should not lead to skipping the remaining sub-tests. Moved the check for *_data() QSKIP()ping to right after *_data() returns, inside the "first global cycle" block that runs it. Previously, this check was done before entering the loop on local data rows, but outside that "first global cycle" block: consequently, later global cycles would fall foul of this check (even though the *_data() hasn't been run in this cycle, much less QSKIP()ped in it) if the last sub-test of the previous global cycle had QSKIP()ped. When running a single test for one specific data row, if the test's *_data() QSKIP()ped, this misplaced check would also have lead to a misleading "Unknown testdata" warning. Changed testlib/selftests' tst_globaldata::skipSingle() to trigger the bug (by having its last local row of first global row skip, which caused the second global row to be omitted) to verify this is also fixed; and amended one of its comments to reflect what's now to be expected. Updated the test's expected output files. Task-number: QTBUG-61774 Change-Id: I99596b595c6d1184038f23383844c6ff51a0cd91 Reviewed-by: Jędrzej Nowacki --- src/testlib/qtestcase.cpp | 78 +++++++++---------- .../selftests/expected_globaldata.lightxml | 31 ++++++-- .../testlib/selftests/expected_globaldata.tap | 23 +++--- .../selftests/expected_globaldata.teamcity | 11 +-- .../testlib/selftests/expected_globaldata.txt | 14 +++- .../testlib/selftests/expected_globaldata.xml | 31 ++++++-- .../selftests/expected_globaldata.xunitxml | 18 ++++- .../selftests/globaldata/tst_globaldata.cpp | 7 +- 8 files changed, 141 insertions(+), 72 deletions(-) diff --git a/src/testlib/qtestcase.cpp b/src/testlib/qtestcase.cpp index d9f8dd78052..a202add2ae5 100644 --- a/src/testlib/qtestcase.cpp +++ b/src/testlib/qtestcase.cpp @@ -1095,7 +1095,7 @@ bool TestMethods::invokeTest(int index, const char *data, WatchDog *watchDog) co const int globalDataCount = gTable->dataCount(); int curGlobalDataIndex = 0; - /* For each test function that has a *_data() table/function, do: */ + /* For each entry in the global data table, do: */ do { if (!gTable->isEmpty()) QTestResult::setCurrentGlobalTestData(gTable->testData(curGlobalDataIndex)); @@ -1103,51 +1103,51 @@ bool TestMethods::invokeTest(int index, const char *data, WatchDog *watchDog) co if (curGlobalDataIndex == 0) { qsnprintf(member, 512, "%s_data()", name.constData()); invokeMethod(QTest::currentTestObject, member); + if (QTestResult::skipCurrentTest()) + break; } bool foundFunction = false; - if (!QTestResult::skipCurrentTest()) { - int curDataIndex = 0; - const int dataCount = table.dataCount(); + int curDataIndex = 0; + const int dataCount = table.dataCount(); - // Data tag requested but none available? - if (data && !dataCount) { - // Let empty data tag through. - if (!*data) - data = 0; - else { - fprintf(stderr, "Unknown testdata for function %s(): '%s'\n", name.constData(), data); - fprintf(stderr, "Function has no testdata.\n"); - return false; - } + // Data tag requested but none available? + if (data && !dataCount) { + // Let empty data tag through. + if (!*data) + data = 0; + else { + fprintf(stderr, "Unknown testdata for function %s(): '%s'\n", name.constData(), data); + fprintf(stderr, "Function has no testdata.\n"); + return false; } - - /* For each entry in the data table, do: */ - do { - QTestResult::setSkipCurrentTest(false); - QTestResult::setBlacklistCurrentTest(false); - if (!data || !qstrcmp(data, table.testData(curDataIndex)->dataTag())) { - foundFunction = true; - - QTestPrivate::checkBlackLists(name.constData(), dataCount ? table.testData(curDataIndex)->dataTag() : 0); - - QTestDataSetter s(curDataIndex >= dataCount ? static_cast(0) - : table.testData(curDataIndex)); - - QTestPrivate::qtestMouseButtons = Qt::NoButton; - if (watchDog) - watchDog->beginTest(); - invokeTestOnData(index); - if (watchDog) - watchDog->testFinished(); - - if (data) - break; - } - ++curDataIndex; - } while (curDataIndex < dataCount); } + /* For each entry in this test's data table, do: */ + do { + QTestResult::setSkipCurrentTest(false); + QTestResult::setBlacklistCurrentTest(false); + if (!data || !qstrcmp(data, table.testData(curDataIndex)->dataTag())) { + foundFunction = true; + + QTestPrivate::checkBlackLists(name.constData(), dataCount ? table.testData(curDataIndex)->dataTag() : 0); + + QTestDataSetter s(curDataIndex >= dataCount ? static_cast(0) + : table.testData(curDataIndex)); + + QTestPrivate::qtestMouseButtons = Qt::NoButton; + if (watchDog) + watchDog->beginTest(); + invokeTestOnData(index); + if (watchDog) + watchDog->testFinished(); + + if (data) + break; + } + ++curDataIndex; + } while (curDataIndex < dataCount); + if (data && !foundFunction) { fprintf(stderr, "Unknown testdata for function %s: '%s()'\n", name.constData(), data); fprintf(stderr, "Available testdata:\n"); diff --git a/tests/auto/testlib/selftests/expected_globaldata.lightxml b/tests/auto/testlib/selftests/expected_globaldata.lightxml index e536031899d..1212e4b3644 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.lightxml +++ b/tests/auto/testlib/selftests/expected_globaldata.lightxml @@ -120,6 +120,30 @@ + + + + + + + + + + + + + + + + + + + + + + + + @@ -143,17 +167,14 @@ - + - + - - - diff --git a/tests/auto/testlib/selftests/expected_globaldata.tap b/tests/auto/testlib/selftests/expected_globaldata.tap index 318299992fd..4d61b7437bd 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.tap +++ b/tests/auto/testlib/selftests/expected_globaldata.tap @@ -29,24 +29,29 @@ ok 7 - skipLocal(global=false:local=false) # SKIP skipping # init skipLocal local=true ok 8 - skipLocal(global=false:local=true) # SKIP skipping # cleanup skipLocal local=true +# init skipLocal local=false +ok 9 - skipLocal(global=true:local=false) # SKIP skipping +# cleanup skipLocal local=false +# init skipLocal local=true +ok 10 - skipLocal(global=true:local=true) # SKIP skipping +# cleanup skipLocal local=true # init skipSingle local=false # global: false local: false # cleanup skipSingle local=false -ok 9 - skipSingle(global=false:local=false) +ok 11 - skipSingle(global=false:local=false) # init skipSingle local=true -# global: false local: true +ok 12 - skipSingle(global=false:local=true) # SKIP Skipping # cleanup skipSingle local=true -ok 10 - skipSingle(global=false:local=true) # init skipSingle local=false -ok 11 - skipSingle(global=true:local=false) # SKIP Skipping +ok 13 - skipSingle(global=true:local=false) # SKIP Skipping # cleanup skipSingle local=false # init skipSingle local=true # global: true local: true # cleanup skipSingle local=true -ok 12 - skipSingle(global=true:local=true) +ok 14 - skipSingle(global=true:local=true) # cleanupTestCase cleanupTestCase (null) -ok 13 - cleanupTestCase() -1..13 -# tests 13 -# pass 9 +ok 15 - cleanupTestCase() +1..15 +# tests 15 +# pass 8 # fail 0 diff --git a/tests/auto/testlib/selftests/expected_globaldata.teamcity b/tests/auto/testlib/selftests/expected_globaldata.teamcity index 2f9f97d9129..f76f6090ba4 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.teamcity +++ b/tests/auto/testlib/selftests/expected_globaldata.teamcity @@ -17,14 +17,15 @@ ##teamcity[testIgnored name='skip()' message='skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] ##teamcity[testIgnored name='skipLocal(local=false)' message='skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] ##teamcity[testIgnored name='skipLocal(local=true)' message='skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] +##teamcity[testIgnored name='skipLocal(local=false)' message='skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] +##teamcity[testIgnored name='skipLocal(local=true)' message='skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] ##teamcity[testStarted name='skipSingle(local=false)' flowId='tst_globaldata'] -##teamcity[testStdOut name='skipSingle(local=false)' out='QDEBUG: init skipLocal local=false|nQDEBUG: cleanup skipLocal local=false|nQDEBUG: init skipLocal local=true|nQDEBUG: cleanup skipLocal local=true|nQDEBUG: init skipSingle local=false|nQDEBUG: global: false local: false|nQDEBUG: cleanup skipSingle local=false' flowId='tst_globaldata'] +##teamcity[testStdOut name='skipSingle(local=false)' out='QDEBUG: init skipLocal local=false|nQDEBUG: cleanup skipLocal local=false|nQDEBUG: init skipLocal local=true|nQDEBUG: cleanup skipLocal local=true|nQDEBUG: init skipLocal local=false|nQDEBUG: cleanup skipLocal local=false|nQDEBUG: init skipLocal local=true|nQDEBUG: cleanup skipLocal local=true|nQDEBUG: init skipSingle local=false|nQDEBUG: global: false local: false|nQDEBUG: cleanup skipSingle local=false' flowId='tst_globaldata'] ##teamcity[testFinished name='skipSingle(local=false)' flowId='tst_globaldata'] -##teamcity[testStarted name='skipSingle(local=true)' flowId='tst_globaldata'] -##teamcity[testStdOut name='skipSingle(local=true)' out='QDEBUG: init skipSingle local=true|nQDEBUG: global: false local: true|nQDEBUG: cleanup skipSingle local=true' flowId='tst_globaldata'] -##teamcity[testFinished name='skipSingle(local=true)' flowId='tst_globaldata'] +##teamcity[testIgnored name='skipSingle(local=true)' message='Skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] ##teamcity[testIgnored name='skipSingle(local=false)' message='Skipping |[Loc: qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)|]' flowId='tst_globaldata'] -##teamcity[testStdOut name='skipSingle(local=true)' out='QDEBUG: init skipSingle local=false|nQDEBUG: cleanup skipSingle local=false|nQDEBUG: init skipSingle local=true|nQDEBUG: global: true local: true|nQDEBUG: cleanup skipSingle local=true' flowId='tst_globaldata'] +##teamcity[testStarted name='skipSingle(local=true)' flowId='tst_globaldata'] +##teamcity[testStdOut name='skipSingle(local=true)' out='QDEBUG: init skipSingle local=true|nQDEBUG: cleanup skipSingle local=true|nQDEBUG: init skipSingle local=false|nQDEBUG: cleanup skipSingle local=false|nQDEBUG: init skipSingle local=true|nQDEBUG: global: true local: true|nQDEBUG: cleanup skipSingle local=true' flowId='tst_globaldata'] ##teamcity[testFinished name='skipSingle(local=true)' flowId='tst_globaldata'] ##teamcity[testStarted name='cleanupTestCase()' flowId='tst_globaldata'] ##teamcity[testStdOut name='cleanupTestCase()' out='QDEBUG: cleanupTestCase cleanupTestCase (null)' flowId='tst_globaldata'] diff --git a/tests/auto/testlib/selftests/expected_globaldata.txt b/tests/auto/testlib/selftests/expected_globaldata.txt index b18568009a6..016b5e82994 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.txt +++ b/tests/auto/testlib/selftests/expected_globaldata.txt @@ -32,14 +32,22 @@ QDEBUG : tst_globaldata::skipLocal(global=false:local=true) init skipLocal local SKIP : tst_globaldata::skipLocal(global=false:local=true) skipping Loc: [qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)] QDEBUG : tst_globaldata::skipLocal(global=false:local=true) cleanup skipLocal local=true +QDEBUG : tst_globaldata::skipLocal(global=true:local=false) init skipLocal local=false +SKIP : tst_globaldata::skipLocal(global=true:local=false) skipping + Loc: [qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)] +QDEBUG : tst_globaldata::skipLocal(global=true:local=false) cleanup skipLocal local=false +QDEBUG : tst_globaldata::skipLocal(global=true:local=true) init skipLocal local=true +SKIP : tst_globaldata::skipLocal(global=true:local=true) skipping + Loc: [qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)] +QDEBUG : tst_globaldata::skipLocal(global=true:local=true) cleanup skipLocal local=true QDEBUG : tst_globaldata::skipSingle(global=false:local=false) init skipSingle local=false QDEBUG : tst_globaldata::skipSingle(global=false:local=false) global: false local: false QDEBUG : tst_globaldata::skipSingle(global=false:local=false) cleanup skipSingle local=false PASS : tst_globaldata::skipSingle(global=false:local=false) QDEBUG : tst_globaldata::skipSingle(global=false:local=true) init skipSingle local=true -QDEBUG : tst_globaldata::skipSingle(global=false:local=true) global: false local: true +SKIP : tst_globaldata::skipSingle(global=false:local=true) Skipping + Loc: [qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)] QDEBUG : tst_globaldata::skipSingle(global=false:local=true) cleanup skipSingle local=true -PASS : tst_globaldata::skipSingle(global=false:local=true) QDEBUG : tst_globaldata::skipSingle(global=true:local=false) init skipSingle local=false SKIP : tst_globaldata::skipSingle(global=true:local=false) Skipping Loc: [qtbase/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp(0)] @@ -50,5 +58,5 @@ QDEBUG : tst_globaldata::skipSingle(global=true:local=true) cleanup skipSingle l PASS : tst_globaldata::skipSingle(global=true:local=true) QDEBUG : tst_globaldata::cleanupTestCase() cleanupTestCase cleanupTestCase (null) PASS : tst_globaldata::cleanupTestCase() -Totals: 9 passed, 0 failed, 4 skipped, 0 blacklisted, 0ms +Totals: 8 passed, 0 failed, 7 skipped, 0 blacklisted, 0ms ********* Finished testing of tst_globaldata ********* diff --git a/tests/auto/testlib/selftests/expected_globaldata.xml b/tests/auto/testlib/selftests/expected_globaldata.xml index b2f92016e58..9aa48c8f161 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.xml +++ b/tests/auto/testlib/selftests/expected_globaldata.xml @@ -122,6 +122,30 @@ + + + + + + + + + + + + + + + + + + + + + + + + @@ -145,17 +169,14 @@ - + - + - - - diff --git a/tests/auto/testlib/selftests/expected_globaldata.xunitxml b/tests/auto/testlib/selftests/expected_globaldata.xunitxml index b135240d26b..de184c2fddc 100644 --- a/tests/auto/testlib/selftests/expected_globaldata.xunitxml +++ b/tests/auto/testlib/selftests/expected_globaldata.xunitxml @@ -1,5 +1,5 @@ - + @@ -36,13 +36,19 @@ + + + + + + - + @@ -79,11 +85,17 @@ + + + + + + - + diff --git a/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp b/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp index f425648ecc9..3c5c2c8f375 100644 --- a/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp +++ b/tests/auto/testlib/selftests/globaldata/tst_globaldata.cpp @@ -125,11 +125,12 @@ void tst_globaldata::skipSingle() QFETCH_GLOBAL(bool, global); QFETCH(bool, local); - // A skip in the last run of one global row suppresses the test in the next - // global row (where a skip in an earlier run of the first row does not). - if (global && !local) + // A skip in the last run of one global row used to suppress the test in the + // next global row (where a skip in an earlier run of the first row did not). + if (global ^ local) QSKIP("Skipping"); qDebug() << "global:" << global << "local:" << local; + QCOMPARE(global, local); } void tst_globaldata::skipLocal()