refs #12167 - moved ordering of markup files into shared code / remov…

…ed related test cases from executor tests (danmar#5642) This is not completely fixing the issue yet. `test-qml.py` still fails when using multiple threads.
chrchr-github · Nov 9, 2023 · b0cde34 · b0cde34
1 parent d24074f
commit b0cde34
Show file tree

Hide file tree

Showing 8 changed files with 122 additions and 172 deletions.
diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp
@@ -186,30 +186,40 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
         logger.printMessage("Please use --suppress for ignoring results from the header files.");
     }
 
-    const std::vector<std::string>& pathnames = parser.getPathNames();
-    const std::list<FileSettings>& fileSettings = parser.getFileSettings();
+    const std::vector<std::string>& pathnamesRef = parser.getPathNames();
+    const std::list<FileSettings>& fileSettingsRef = parser.getFileSettings();
 
     // the inputs can only be used exclusively - CmdLineParser should already handle this
-    assert(!(!pathnames.empty() && !fileSettings.empty()));
+    assert(!(!pathnamesRef.empty() && !fileSettingsRef.empty()));
 
-    if (!fileSettings.empty()) {
+    if (!fileSettingsRef.empty()) {
+        std::list<FileSettings> fileSettings;
         if (!settings.fileFilters.empty()) {
             // filter only for the selected filenames from all project files
-            std::copy_if(fileSettings.cbegin(), fileSettings.cend(), std::back_inserter(mFileSettings), [&](const FileSettings &fs) {
+            std::copy_if(fileSettingsRef.cbegin(), fileSettingsRef.cend(), std::back_inserter(fileSettings), [&](const FileSettings &fs) {
                 return matchglobs(settings.fileFilters, fs.filename);
             });
-            if (mFileSettings.empty()) {
+            if (fileSettings.empty()) {
                 logger.printError("could not find any files matching the filter.");
                 return false;
             }
         }
         else {
-            mFileSettings = fileSettings;
+            fileSettings = fileSettingsRef;
         }
+
+        // sort the markup last
+        std::copy_if(fileSettings.cbegin(), fileSettings.cend(), std::back_inserter(mFileSettings), [&](const FileSettings &fs) {
+            return !settings.library.markupFile(fs.filename) || !settings.library.processMarkupAfterCode(fs.filename);
+        });
+
+        std::copy_if(fileSettings.cbegin(), fileSettings.cend(), std::back_inserter(mFileSettings), [&](const FileSettings &fs) {
+            return settings.library.markupFile(fs.filename) && settings.library.processMarkupAfterCode(fs.filename);
+        });
     }
 
-    if (!pathnames.empty()) {
-        std::list<std::pair<std::string, std::size_t>> files;
+    if (!pathnamesRef.empty()) {
+        std::list<std::pair<std::string, std::size_t>> filesResolved;
         // TODO: this needs to be inlined into PathMatch as it depends on the underlying filesystem
 #if defined(_WIN32)
         // For Windows we want case-insensitive path matching
@@ -219,26 +229,36 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
 #endif
         // Execute recursiveAddFiles() to each given file parameter
         const PathMatch matcher(ignored, caseSensitive);
-        for (const std::string &pathname : pathnames) {
-            const std::string err = FileLister::recursiveAddFiles(files, Path::toNativeSeparators(pathname), settings.library.markupExtensions(), matcher);
+        for (const std::string &pathname : pathnamesRef) {
+            const std::string err = FileLister::recursiveAddFiles(filesResolved, Path::toNativeSeparators(pathname), settings.library.markupExtensions(), matcher);
             if (!err.empty()) {
                 // TODO: bail out?
                 logger.printMessage(err);
             }
         }
 
+        std::list<std::pair<std::string, std::size_t>> files;
         if (!settings.fileFilters.empty()) {
-            std::copy_if(files.cbegin(), files.cend(), std::inserter(mFiles, mFiles.end()), [&](const decltype(files)::value_type& entry) {
+            std::copy_if(filesResolved.cbegin(), filesResolved.cend(), std::inserter(files, files.end()), [&](const decltype(filesResolved)::value_type& entry) {
                 return matchglobs(settings.fileFilters, entry.first);
             });
-            if (mFiles.empty()) {
+            if (files.empty()) {
                 logger.printError("could not find any files matching the filter.");
                 return false;
             }
         }
         else {
-            mFiles = files;
+            files = std::move(filesResolved);
         }
+
+        // sort the markup last
+        std::copy_if(files.cbegin(), files.cend(), std::inserter(mFiles, mFiles.end()), [&](const decltype(files)::value_type& entry) {
+            return !settings.library.markupFile(entry.first) || !settings.library.processMarkupAfterCode(entry.first);
+        });
+
+        std::copy_if(files.cbegin(), files.cend(), std::inserter(mFiles, mFiles.end()), [&](const decltype(files)::value_type& entry) {
+            return settings.library.markupFile(entry.first) && settings.library.processMarkupAfterCode(entry.first);
+        });
     }
 
     if (mFiles.empty() && mFileSettings.empty()) {

diff --git a/cli/singleexecutor.cpp b/cli/singleexecutor.cpp
@@ -51,52 +51,23 @@ unsigned int SingleExecutor::check()
     unsigned int c = 0;
 
     for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
-        if (!mSettings.library.markupFile(i->first) || !mSettings.library.processMarkupAfterCode(i->first)) {
-            result += mCppcheck.check(i->first);
-            processedsize += i->second;
-            ++c;
-            if (!mSettings.quiet)
-                reportStatus(c, mFiles.size(), processedsize, totalfilesize);
-            // TODO: call analyseClangTidy()?
-        }
+        result += mCppcheck.check(i->first);
+        processedsize += i->second;
+        ++c;
+        if (!mSettings.quiet)
+            reportStatus(c, mFiles.size(), processedsize, totalfilesize);
+        // TODO: call analyseClangTidy()?
     }
 
     // filesettings
     // check all files of the project
     for (const FileSettings &fs : mFileSettings) {
-        if (!mSettings.library.markupFile(fs.filename) || !mSettings.library.processMarkupAfterCode(fs.filename)) {
-            result += mCppcheck.check(fs);
-            ++c;
-            if (!mSettings.quiet)
-                reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
-            if (mSettings.clangTidy)
-                mCppcheck.analyseClangTidy(fs);
-        }
-    }
-
-    // second loop to parse all markup files which may not work until all
-    // c/cpp files have been parsed and checked
-    // TODO: get rid of duplicated code
-    for (std::list<std::pair<std::string, std::size_t>>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
-        if (mSettings.library.markupFile(i->first) && mSettings.library.processMarkupAfterCode(i->first)) {
-            result += mCppcheck.check(i->first);
-            processedsize += i->second;
-            ++c;
-            if (!mSettings.quiet)
-                reportStatus(c, mFiles.size(), processedsize, totalfilesize);
-            // TODO: call analyseClangTidy()?
-        }
-    }
-
-    for (const FileSettings &fs : mFileSettings) {
-        if (mSettings.library.markupFile(fs.filename) && mSettings.library.processMarkupAfterCode(fs.filename)) {
-            result += mCppcheck.check(fs);
-            ++c;
-            if (!mSettings.quiet)
-                reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
-            if (mSettings.clangTidy)
-                mCppcheck.analyseClangTidy(fs);
-        }
+        result += mCppcheck.check(fs);
+        ++c;
+        if (!mSettings.quiet)
+            reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
+        if (mSettings.clangTidy)
+            mCppcheck.analyseClangTidy(fs);
     }
 
     if (mCppcheck.analyseWholeProgram())

diff --git a/test/cli/test-other.py b/test/cli/test-other.py
@@ -664,3 +664,63 @@ def test_file_order(tmpdir):
         '4/4 files checked 0% done'
     ]
     assert stderr == ''
+
+
+def test_markup(tmpdir):
+    test_file_1 = os.path.join(tmpdir, 'test_1.qml')
+    with open(test_file_1, 'wt') as f:
+        pass
+    test_file_2 = os.path.join(tmpdir, 'test_2.cpp')
+    with open(test_file_2, 'wt') as f:
+        pass
+    test_file_3 = os.path.join(tmpdir, 'test_3.qml')
+    with open(test_file_3, 'wt') as f:
+        pass
+    test_file_4 = os.path.join(tmpdir, 'test_4.cpp')
+    with open(test_file_4, 'wt') as f:
+        pass
+
+    args = ['--library=qt', test_file_1, test_file_2, test_file_3, test_file_4]
+    out_lines = [
+        'Checking {} ...'.format(test_file_2),
+        '1/4 files checked 0% done',
+        'Checking {} ...'.format(test_file_4),
+        '2/4 files checked 0% done',
+        'Checking {} ...'.format(test_file_1),
+        '3/4 files checked 0% done',
+        'Checking {} ...'.format(test_file_3),
+        '4/4 files checked 0% done'
+    ]
+
+    assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines)
+
+
+def test_markup_j(tmpdir):
+    test_file_1 = os.path.join(tmpdir, 'test_1.qml')
+    with open(test_file_1, 'wt') as f:
+        pass
+    test_file_2 = os.path.join(tmpdir, 'test_2.cpp')
+    with open(test_file_2, 'wt') as f:
+        pass
+    test_file_3 = os.path.join(tmpdir, 'test_3.qml')
+    with open(test_file_3, 'wt') as f:
+        pass
+    test_file_4 = os.path.join(tmpdir, 'test_4.cpp')
+    with open(test_file_4, 'wt') as f:
+        pass
+
+    args = ['--library=qt', test_file_1, test_file_2, test_file_3, test_file_4]
+
+    exitcode, stdout, stderr = cppcheck(args)
+    assert exitcode == 0
+    lines = stdout.splitlines()
+    for i in range(1, 5):
+        lines.remove('{}/4 files checked 0% done'.format(i))
+
+    assert lines == [
+        'Checking {} ...'.format(test_file_2),
+        'Checking {} ...'.format(test_file_4),
+        'Checking {} ...'.format(test_file_1),
+        'Checking {} ...'.format(test_file_3)
+    ]
+    assert stderr == ''
diff --git a/test/cli/test-qml.py b/test/cli/test-qml.py
@@ -2,13 +2,27 @@
 # python3 -m pytest test-qml.py
 
 import os
+import pytest
 from testutils import cppcheck
 
 PROJECT_DIR = 'QML-Samples-TableView'
 
+
 def test_unused_functions():
     ret, stdout, stderr = cppcheck(['--library=qt', '--enable=unusedFunction', PROJECT_DIR])
     # there are unused functions. But fillSampleData is not unused because that is referenced from main.qml
     assert '[unusedFunction]' in stderr
     assert 'fillSampleData' not in stderr
 
+
+@pytest.mark.xfail
+def test_unused_functions_j(tmpdir):
+    build_dir = os.path.join(tmpdir, 'b1')
+    os.mkdir(build_dir)
+    ret, stdout, stderr = cppcheck(['--library=qt', '--enable=unusedFunction', '-j2', '--cppcheck-build-dir={}'.format(build_dir), PROJECT_DIR])
+    # there are unused functions. But fillSampleData is not unused because that is referenced from main.qml
+    assert '[unusedFunction]' in stderr
+    assert 'fillSampleData' not in stderr
+
+# TODO: test with project file
+# TODO: test with FileSettings
diff --git a/test/cli/testutils.py b/test/cli/testutils.py
@@ -1,4 +1,3 @@
-
 import logging
 import os
 import subprocess
@@ -83,7 +82,7 @@ def assert_cppcheck(args, ec_exp=None, out_exp=None, err_exp=None, env=None):
         assert exitcode == ec_exp, stdout
     if out_exp is not None:
         out_lines = stdout.splitlines()
-        assert out_lines == out_exp
+        assert out_lines == out_exp, stdout
     if err_exp is not None:
         err_lines = stderr.splitlines()
-        assert err_lines == err_exp
+        assert err_lines == err_exp, stderr
diff --git a/test/testprocessexecutor.cpp b/test/testprocessexecutor.cpp
@@ -147,7 +147,6 @@ class TestProcessExecutorBase : public TestFixture {
         TEST_CASE(no_errors_equal_amount_files);
         TEST_CASE(one_error_less_files);
         TEST_CASE(one_error_several_files);
-        TEST_CASE(markup);
         TEST_CASE(clangTidy);
         TEST_CASE(showtime_top5_file);
         TEST_CASE(showtime_top5_summary);
@@ -244,46 +243,6 @@ class TestProcessExecutorBase : public TestFixture {
               "}");
     }
 
-    void markup() {
-        const Settings settingsOld = settings;
-        settings.library.mMarkupExtensions.emplace(".cp1");
-        settings.library.mProcessAfterCode.emplace(".cp1", true);
-
-        const std::vector<std::string> files = {
-            fprefix() + "_1.cp1", fprefix() + "_2.cpp", fprefix() + "_3.cp1", fprefix() + "_4.cpp"
-        };
-
-        // the checks are not executed on the markup files => expected result is 2
-        check(2, 4, 2,
-              "int main()\n"
-              "{\n"
-              "  int i = *((int*)0);\n"
-              "  return 0;\n"
-              "}",
-              dinit(CheckOptions,
-                    $.quiet = false,
-                        $.filesList = files));
-        // TODO: order of "Checking" and "checked" is affected by thread
-        /*TODO_ASSERT_EQUALS("Checking " + fprefix() + "_2.cpp ...\n"
-                           "1/4 files checked 25% done\n"
-                           "Checking " + fprefix() + "_4.cpp ...\n"
-                           "2/4 files checked 50% done\n"
-                           "Checking " + fprefix() + "_1.cp1 ...\n"
-                           "3/4 files checked 75% done\n"
-                           "Checking " + fprefix() + "_3.cp1 ...\n"
-                           "4/4 files checked 100% done\n",
-                           "Checking " + fprefix() + "_1.cp1 ...\n"
-                           "1/4 files checked 25% done\n"
-                           "Checking " + fprefix() + "_2.cpp ...\n"
-                           "2/4 files checked 50% done\n"
-                           "Checking " + fprefix() + "_3.cp1 ...\n"
-                           "3/4 files checked 75% done\n"
-                           "Checking " + fprefix() + "_4.cpp ...\n"
-                           "4/4 files checked 100% done\n",
-                           output.str());*/
-        settings = settingsOld;
-    }
-
     void clangTidy() {
         // TODO: we currently only invoke it with ImportProject::FileSettings
         if (!useFS)

diff --git a/test/testsingleexecutor.cpp b/test/testsingleexecutor.cpp
@@ -151,7 +151,6 @@ class TestSingleExecutorBase : public TestFixture {
         TEST_CASE(no_errors_equal_amount_files);
         TEST_CASE(one_error_less_files);
         TEST_CASE(one_error_several_files);
-        TEST_CASE(markup);
         TEST_CASE(clangTidy);
         TEST_CASE(showtime_top5_file);
         TEST_CASE(showtime_top5_summary);
@@ -240,37 +239,6 @@ class TestSingleExecutorBase : public TestFixture {
               "}");
     }
 
-    void markup() {
-        const Settings settingsOld = settings;
-        settings.library.mMarkupExtensions.emplace(".cp1");
-        settings.library.mProcessAfterCode.emplace(".cp1", true);
-
-        const std::vector<std::string> files = {
-            fprefix() + "_1.cp1", fprefix() + "_2.cpp", fprefix() + "_3.cp1", fprefix() + "_4.cpp"
-        };
-
-        // checks are not executed on markup files => expected result is 2
-        check(4, 2,
-              "int main()\n"
-              "{\n"
-              "  int i = *((int*)0);\n"
-              "  return 0;\n"
-              "}",
-              dinit(CheckOptions,
-                    $.quiet = false,
-                        $.filesList = files));
-        // TODO: filter out the "files checked" messages
-        ASSERT_EQUALS("Checking " + fprefix() + "_2.cpp ...\n"
-                      "1/4 files checked 25% done\n"
-                      "Checking " + fprefix() + "_4.cpp ...\n"
-                      "2/4 files checked 50% done\n"
-                      "Checking " + fprefix() + "_1.cp1 ...\n"
-                      "3/4 files checked 75% done\n"
-                      "Checking " + fprefix() + "_3.cp1 ...\n"
-                      "4/4 files checked 100% done\n", output.str());
-        settings = settingsOld;
-    }
-
     void clangTidy() {
         // TODO: we currently only invoke it with ImportProject::FileSettings
         if (!useFS)