Fix #13523 CTU: Mention including cpp file for error in header #7174

chrchr-github · 2025-01-03T22:35:46Z

No description provided.

chrchr-github · 2025-01-04T10:14:39Z

test/cli/whole-program_test.py

+    file0 = ''
+    for e in results.findall('errors/error'):
+        if e.attrib['id'] == 'ctunullpointer':
+            if 'file0' in e.attrib:


Not sure why this is necessary, but there was a KeyError without it.

That means that it doesn't contain that key. [] acts like at()in C++.

Yeah, but there should only be one ctunullpointer error, which has the file0 attribute.
XML shown here: https://github.com/danmar/cppcheck/actions/runs/12605458944/job/35134122531
KeyError: https://github.com/danmar/cppcheck/actions/runs/12605258027/job/35133607619

We should probably add a few more asserts on the results. Like the amount of returned nodes and such.

You could also limit the XPath to only return the node in question by adding [@id='ctunullpointer'].

We should probably add a few more asserts on the results. Like the amount of returned nodes and such.

Not so easy, since getting both nullpointer and ctunullpointer seems wrong already. With -j2, we get an additional ctunullpointer (https://trac.cppcheck.net/ticket/11883)

You could also limit the XPath to only return the node in question by adding [@id='ctunullpointer'].

Done.

lib/checkclass.cpp

test/cli/whole-program_test.py

firewave · 2025-01-04T22:03:18Z

FYI running the tests locally on Windows is actually quite easy. You can install the official Python distribution from the Microsoft Store (https://apps.microsoft.com/detail/9pnrbtzxmb4z) which will place python in the PATH and then you can just run them from any command-line.

lib/checkbufferoverrun.cpp

chrchr-github · 2025-01-04T22:24:30Z

FYI running the tests locally on Windows is actually quite easy. You can install the official Python distribution from the Microsoft Store (https://apps.microsoft.com/detail/9pnrbtzxmb4z) which will place python in the PATH and then you can just run them from any command-line.

Hadn't even tried that, fearing another rabbit hole. But it works:

python -m pytest .\whole-program_test.py
================================================= test session starts =================================================
platform win32 -- Python 3.10.2, pytest-8.3.4, pluggy-1.5.0
rootdir: source\repos\cppcheck\test\cli
collected 31 items

whole-program_test.py .x.x.x.x.x.x.x.x......x...x...x                                                            [100%]

=========================================== 20 passed, 11 xfailed in 43.52s ===========================================

danmar

I am not strongly against that we test it in test/cli but I wonder why it wasn't tested in testrunner instead.
A pytest that executes cppcheck is slower than a testrunner test as far as I understand?

firewave · 2025-01-05T17:04:18Z

A pytest that executes cppcheck is slower than a testrunner test as far as I understand?

Obviously by design but all the Python tests together take less than 40 seconds. So even if we add several dozens more that will hardly make a dent in the CI runtime.

firewave · 2025-01-05T17:07:58Z

I am not strongly against that we test it in test/cli but I wonder why it wasn't tested in testrunner instead.

We also get all the injection matrix in the CI with those tests which is actually important as these behave different with threads and/or builddir. And that is annoying or really hard to test in the unit tests and here we get it for "free".

chrchr-github · 2025-01-07T17:00:48Z

Not sure why test_nullpointer_file0_j suddenly passed and then failed again (it always fails locally).

___________________________ test_nullpointer_file0_j ___________________________
[gw0] linux -- Python 3.11.11 /opt/hostedtoolcache/Python/3.11.11/x64/bin/python3
[XPASS(strict)]

https://github.com/danmar/cppcheck/actions/runs/12654460990/job/35262407668
https://github.com/danmar/cppcheck/actions/runs/12654913760/job/35263940012

firewave · 2025-01-07T17:27:35Z

test/cli/whole-program_test.py

+    assert file0 == 'whole-program/nullpointer1.cpp'
+    assert stdout == ''
+    assert ret == 1, stdout
+
+
+def test_nullpointer_file0():
+    __test_nullpointer_file0(['-j1'])
+
+@pytest.mark.skip(reason="flaky!?")


Suggested change

assert file0 == 'whole-program/nullpointer1.cpp'

assert stdout == ''

assert ret == 1, stdout

def test_nullpointer_file0():

__test_nullpointer_file0(['-j1'])

@pytest.mark.skip(reason="flaky!?")

assert ret == 1, stdout if stdout else stderr

assert stdout == ''

assert file0 == 'whole-program/nullpointer1.cpp', stderr

def test_nullpointer_file0():

__test_nullpointer_file0(['-j1'])

Usually I would assume it is an issue with running the test in parallel but there is nothing in this test which could cause that.

But this test is build with UBSAN so it might indicate such an issue. The asserts are in a bad order so that would be obfuscated. I adjusted them so we should get more details (that needs to be applied across all the tests - there are already some initial helpers but something for later).

The xfail is still in, so it might pass now but we might not see the error reported by the sanitizer.

With xfail in, Run test/cli (--cppcheck-build-dir) fails (test succeeds). Without xfail, Run test/cli fails as expected.
How is this injection stuff supposed to work? I also see extraargs being used in the test.

The injection stuff checks if any of the parameters in question already exists and only injects those is necessary. The -j2 injection will not be done if -j1 already exists. Same with --cppcheck-build-dir and --executor. So if you have to specify them explicitly they will not return the same no matter how it is called (e.g. Windows does not support --executor=process) and you need to provide an override.

This looks weird though. Will take a look later.

chrchr-github added 8 commits January 3, 2025 23:32

Fix #13523 Mention including cpp file for error in header

11a1f22

Fix

f864c2b

Fix

bf3a994

Fix

f6b31de

Fix

46239d5

debug

12b2dd0

debug

1ab20e6

undo

791b066

chrchr-github marked this pull request as ready for review January 4, 2025 10:13

chrchr-github commented Jan 4, 2025

View reviewed changes

lib/checkclass.cpp Outdated Show resolved Hide resolved

chrchr-github added 2 commits January 4, 2025 11:34

Refactor

e04d0a6

Format [skip ci]

706c5ca

firewave reviewed Jan 4, 2025

View reviewed changes

test/cli/whole-program_test.py Show resolved Hide resolved

chrchr-github added 3 commits January 4, 2025 12:04

Add -j2

9a57c31

Add file0 in remaining CTU checks

e0a623c

xfail

e28646c

chrchr-github changed the title ~~Fix #13523 Mention including cpp file for error in header~~ Fix #13523 CTU: Mention including cpp file for error in header Jan 4, 2025

Use build dir with -j2

65e54b5

firewave reviewed Jan 4, 2025

View reviewed changes

test/cli/whole-program_test.py Outdated Show resolved Hide resolved

chrchr-github added 2 commits January 4, 2025 19:59

Fix, simplify

25d1801

xfail

7309cf3

firewave reviewed Jan 4, 2025

View reviewed changes

lib/checkbufferoverrun.cpp Show resolved Hide resolved

firewave approved these changes Jan 5, 2025

View reviewed changes

danmar reviewed Jan 5, 2025

View reviewed changes

chrchr-github closed this Jan 7, 2025

chrchr-github added 2 commits January 7, 2025 16:40

Update cppcheck.cpp

9a41138

Update whole-program_test.py

fbe7f87

chrchr-github reopened this Jan 7, 2025

chrchr-github added 3 commits January 7, 2025 17:08

Update whole-program_test.py

1a219d8

Update whole-program_test.py

a9b9de4

Update whole-program_test.py

750b92c

firewave reviewed Jan 7, 2025

View reviewed changes

chrchr-github added 6 commits January 7, 2025 18:39

Update whole-program_test.py

f65bc60

Update whole-program_test.py

33cedb5

Update whole-program_test.py

cc60882

Update whole-program_test.py

4f288ff

Update whole-program_test.py

155eea9

Update whole-program_test.py

e6c30a2

chrchr-github merged commit 77a6705 into danmar:main Jan 8, 2025
60 checks passed

chrchr-github deleted the chr_13523 branch January 8, 2025 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #13523 CTU: Mention including cpp file for error in header #7174

Fix #13523 CTU: Mention including cpp file for error in header #7174

chrchr-github commented Jan 3, 2025

chrchr-github Jan 4, 2025

firewave Jan 4, 2025

chrchr-github Jan 4, 2025

firewave Jan 4, 2025

chrchr-github Jan 4, 2025

firewave commented Jan 4, 2025

chrchr-github commented Jan 4, 2025

danmar left a comment

firewave commented Jan 5, 2025

firewave commented Jan 5, 2025

chrchr-github commented Jan 7, 2025

firewave Jan 7, 2025 •

edited

Loading

firewave Jan 7, 2025

firewave Jan 7, 2025

chrchr-github Jan 7, 2025

firewave Jan 7, 2025

Fix #13523 CTU: Mention including cpp file for error in header #7174

Fix #13523 CTU: Mention including cpp file for error in header #7174

Conversation

chrchr-github commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firewave commented Jan 4, 2025

chrchr-github commented Jan 4, 2025

danmar left a comment

Choose a reason for hiding this comment

firewave commented Jan 5, 2025

firewave commented Jan 5, 2025

chrchr-github commented Jan 7, 2025

firewave Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

firewave Jan 7, 2025 •

edited

Loading