ASSERT() on calls to Tokenizer::tokenize() in test code by chrchr-github · Pull Request #3501 · cppcheck-opensource/cppcheck

chrchr-github · 2021-10-11T17:36:24Z

No description provided.

chrchr-github · 2021-10-15T18:33:04Z

And how about this one? ;)

danmar · 2021-10-15T18:49:10Z

        LOAD_LIB_2(settings.library, "std.cfg");
        std::istringstream istr(code);
-        tokenizer.tokenize(istr, "test.cpp");
+        ASSERT(tokenizer.tokenize(istr, "test.cpp"));


The idea is pretty good. We want to fail if this fails. Unfortunately the output will not make much sense. Can we do something alternative that will make the output somewhat more helpful if this fails.

Hmm, like a macro ASSERT_TOKENIZE() that adds something to errout on failure? Or what do you have in mind?

I don't think ASSERT_TOKENIZE will work neither it would point out the line where the tokenize fails right? So you have no idea where check is called from.
How about:

if (!tokenizer.tokenize(istr, "test.cpp)) errout << "tokenize failed!";

So execution will continue and when errout is checked there will be an assertion error that is more clear.
An alternative could be to change check() so it takes a line number and then we can print the line number in the error message.

#define check(code) check_(code, __LINE__) void check_(const char code[], int line) .....

What I want is that if the test case at line 654 calls check() directly or indirectly. Then I want that the assertion message says that the test case at line 654 fails. I fear that it will complain about line 53 with your suggestion where the tokenizer.tokenize() is called. And then you have to manually debug to figure out where the problem is.

That would require numerous changes, since every test defines its own check()-like functions... Could be done though.

That would require numerous changes, since every test defines its own check()-like functions... Could be done though.

could you try it out on 1 check function to start with so we see that it looks reasonable.

I have added something to test64bit.cpp

firewave · 2021-10-15T19:28:47Z

        Tokenizer tokenizer(&settings, this);
        std::istringstream istr(code);
-        tokenizer.tokenize(istr, "test.cpp");
+        ASSERT(tokenizer.tokenize(istr, "test.cpp")) false;


That looks some search and replace bug - there's more of that.

ASSERT needs to return something here, Maybe that's not the way it's supposed to be used though...

yes this looks weird.

firewave · 2021-10-15T19:29:02Z

        Tokenizer tokenizer(&settings, this);
        std::istringstream istr(code);
-        tokenizer.tokenize(istr, "test.cpp");
+        ASSERT(tokenizer.tokenize(istr, "test.cpp")) {};


That looks some search and replace bug - there's more of that.

…/cppcheck into chr_AssertTokenize

danmar

Nice! It looks promising

danmar · 2021-10-27T06:40:25Z


 #define TEST_CASE( NAME )  do { if (prepareTest(#NAME)) { setVerbose(false); NAME(); } } while (false)
 #define ASSERT( CONDITION )  if (!assert_(__FILE__, __LINE__, (CONDITION))) return
+#define ASSERT_LOC( CONDITION )  if (!assert_(file, line, (CONDITION))) return


I would prefer that the macro takes the file,line as arguments so the interface is clear.

I don't think a return here will help much, the test will continue to run. Probably a very misleading assertion failure would follow. I suggest that the return is removed and the test will continue as before.

chrchr-github · 2021-10-28T22:01:48Z

There is quite a number of check()-like functions in some of the files, so this could get tedious. Anyway, I have found another broken test case, see the latest commit.

danmar · 2021-10-29T05:22:11Z

There is quite a number of check()-like functions in some of the files, so this could get tedious

maybe we can merge it step by step. If you clean this up I would not mind to merge it.

danmar · 2021-10-29T05:23:00Z

Anyway, I have found another broken test case, see the latest commit.

good 👍

danmar · 2021-10-30T07:17:42Z

        Tokenizer tokenizer(&settings, this);
        std::istringstream istr(code);
-        tokenizer.tokenize(istr, "test.cpp");
+        ASSERT(tokenizer.tokenize(istr, "test.cpp")) false;


before I merge I want that this is fixed.

Ok, there is still a lot to do yet. You could also push this separately: fa69d3c

I pushed that with f363d89

…/cppcheck into chr_AssertTokenize

chrchr-github · 2021-11-18T11:21:56Z

I think I'm more or less done now. Feel free to merge anything you like...

chrchr-github and others added 6 commits October 11, 2021 10:47

ASSERT() on calls to Tokenizer::tokenize() in test code

1417eba

Fix return type

fecec17

Fix return type

2d3dfeb

Merge branch 'main' into chr_AssertTokenize

4a625c0

Missing assert

dbedbd4

Merge branch 'danmar:main' into chr_AssertTokenize

233dbeb

danmar reviewed Oct 15, 2021

View reviewed changes

firewave reviewed Oct 15, 2021

View reviewed changes

chrchr-github and others added 4 commits October 26, 2021 17:41

Merge

2f90454

Merge branch 'chr_AssertTokenize' of https://github.com/chrchr-github…

7c913aa

…/cppcheck into chr_AssertTokenize

Add ASSERT_LOC() to capture file/line info

e6c47bd

Fix test case

b28dbb5

danmar reviewed Oct 27, 2021

View reviewed changes

chrchr-github added 3 commits October 27, 2021 09:37

Add args to ASSERT_LOC(), no return

f8174f4

Add macros to capture file/line

9ea857b

Fix test case

fa69d3c

chrchr-github added 3 commits October 29, 2021 19:47

More ASSERT_LOC

a7e8184

Unused parameters

1a66371

Merge branch 'main' into chr_AssertTokenize

1f5fccb

danmar reviewed Oct 30, 2021

View reviewed changes

chrchr-github and others added 5 commits November 2, 2021 19:57

More ASSERT_LOC

6d476c2

Merge branch 'main' into chr_AssertTokenize

4996985

ASSERT_LOC

639f45e

Merge branch 'chr_AssertTokenize' of https://github.com/chrchr-github…

4d87089

…/cppcheck into chr_AssertTokenize

format

17b6484

chrchr-github added 5 commits November 3, 2021 10:20

format

96a0512

More ASSERT_LOC()

dbf0e71

More ASSERT_LOC, ASSERT

a8f2d11

Merge branch 'main' into chr_AssertTokenize

5277004

More ASSERT_LOC

6d204b0

danmar merged commit ca311eb into cppcheck-opensource:main Nov 29, 2021

chrchr-github deleted the chr_AssertTokenize branch April 10, 2022 21:15

Conversation

chrchr-github commented Oct 11, 2021

Uh oh!

chrchr-github commented Oct 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Oct 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrchr-github commented Oct 28, 2021

Uh oh!

danmar commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmar commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrchr-github commented Nov 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmar Oct 16, 2021 •

edited

Loading

danmar commented Oct 29, 2021 •

edited

Loading