-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed #11883 - CTU path information from build dir was not being used #7202
base: main
Are you sure you want to change the base?
Conversation
__test_addon_suppr(tmp_path, ['--suppress=misra-c2012-2.3:{}:3'.format(test_file)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be in whole-program_test.py
? (referring to test_ctu_path_builddir
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically it does. But (at least in my book) the grouping of those tests is around the whole-program
folder in the repo (similar to proj2
, helloword
, etc.).
But other_test.py
should probably to be split up at some point as it just a dumping ground for random tests. But that is not something I am willing to spend any time on soon with all the pending stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weeell... You could add a file in whole-program
instead of creating it on-the-fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would start another dumping ground. IMO we should only create physical files if necessary (headers, folders, ...) or if they have a certain complexity/size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I'd still prefer the new test in whole-program_test.py
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would start another dumping ground.
One one side I also spontanously think all tests related to whole program analysis should be in the same test file. But I can understand your logic too.
IMO we should only create physical files if necessary
Certainly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not something you should do here in this PR.. it's just an observation.
The other_test.py has many tests related to suppressions and therefore I feel it would make sense to create a testfile for all suppression tests.
|
||
exitcode_1, stdout_1, stderr_1 = cppcheck(args) | ||
print(stderr_1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho this print should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - an accidental leftover. Will be removed in the follow-up.
No description provided.