add command line option to Run ignore test #908

cuitoldfish · 2016-03-20T15:20:22Z

using "-runIgnore" to run IGNORE_TEST

coveralls · 2016-03-21T10:17:44Z

Coverage increased (+0.0003%) to 99.907% when pulling a5de964 on cuitoldfish:runIgnoreTest into ea21a28 on cpputest:master.

basvodde · 2016-03-21T10:41:49Z

include/CppUTest/CommandLineArguments.h

@@ -45,6 +45,7 @@ class CommandLineArguments
    bool isColor() const;
    bool isListingTestGroupNames() const;
    bool isListingTestGroupAndCaseNames() const;
+	bool isRunIgnore() const;


Hey, whats with the spacing? :)

basvodde · 2016-03-21T10:45:44Z

I'd suggest to make the option -i (or so) and make the longer option --ignore (with double-dash). That is kinda the practice in unix apps. Or just have the -i option.

arstrube · 2016-03-21T12:41:07Z

Hey @cuitoldfish you spooked the Dos build :D. One of its 6 part-exes ran out of memory linking. @basvodde Guess we'll just have to drop the tests, hehe ;-)

basvodde · 2016-03-21T12:42:09Z

7th part :)

cuitoldfish · 2016-03-21T12:49:57Z

hey, only "-i" option can describe clearly?

what's the failure? I have slow network and could not response to those failure immediately. what's does this "ran out of memory linking" mean? I didn't do any thing to consume the size.

arstrube · 2016-03-21T13:32:15Z

@cuitoldfish well you didnt. but your code did ;-).
@basvodde - that means lots of shunting around :-/ but probably the best thing to do, so it will take longer before we run out of space again.....

cuitoldfish · 2016-03-21T13:56:19Z

@arstrube, so seems I can not pass dos build unless you drop those tests files? :)

@basvodde, I prefer "-ri" if clean is the rule for command line option

basvodde · 2016-03-21T13:58:18Z

-ri will be fine. Not sure why the r ?

arstrube · 2016-03-21T14:14:52Z

@cuitoldfish yeah, unless you feel like fiddling with platforms\Dos\sources.mk yourself a little ;-)

cuitoldfish · 2016-03-21T15:01:07Z

—ri is the abbreviation of run ignore, i think it is easier to remember

basvodde · 2016-03-22T00:06:42Z

Ah run :) Ok

basvodde · 2016-03-22T00:09:23Z

The rest looks good. But would appreciate fixing the formatting. If you look at the pull request diff, you can see it looks kinda ugly now :(

cuitoldfish · 2016-03-22T01:59:12Z

Oh, I forget to mention that formatting error. That is because wrong configuration in my editor. I have corrected those. Will commit soon

coveralls · 2016-03-22T06:35:25Z

Coverage increased (+0.0003%) to 99.907% when pulling bbf318b on cuitoldfish:runIgnoreTest into ea21a28 on cpputest:master.

basvodde · 2016-03-22T10:28:51Z

include/CppUTest/CommandLineArguments.h

@@ -69,6 +70,7 @@ class CommandLineArguments
    bool runTestsAsSeperateProcess_;
    bool listTestGroupNames_;
    bool listTestGroupAndCaseNames_;
+	bool runIgnore_;


yeah, I'm sorry for this missing one, thanks for your reminder!

basvodde · 2016-03-22T10:35:32Z

Another simplification. I'm a bit annoyed by the "setRunIgnored" flag, but haven't figured out yet how to get rid of that. But at least, you can remove the _runIgnored member field out of the UtestShell and only have it in the IgnoredShell...

Then you can also remove the isIgnored from the UtestShell interface.

cuitoldfish · 2016-03-22T12:19:37Z

the current implementation in TestRegistry program to the UtestShell interface, if isRunIgnored only exist IgnoredShell, then how to call it from TestRegistry?

basvodde · 2016-03-22T12:30:01Z

:)

It isn't called from TestRegistry as far as I can see :)

cuitoldfish · 2016-03-23T01:10:16Z

src/CppUTest/TestRegistry.cpp

@@ -50,6 +50,7 @@ void TestRegistry::runAllTests(TestResult& result)
    result.testsStarted();
    for (UtestShell *test = tests_; test != NULL; test = test->getNext()) {


@basvodde , here we can see, in the TestRegistry, all test shell is taken to process one by one based on the UtestShell interface. so i need to add setRunIgnore in the UtestShell interface, or I have to segregate the UtestShell interface to get a few new classification

@basvodde it is in fact a new interface we are adding to UtestShell, and that's why in my opinion it needs to be there (like @cuitoldfish pointed out). It is the same as runInSeparateProcess_ or currentRepetition_. We need to be able to tell UtestShell from the "outside world" how it is supposed to behave, e.g. test->setRunIgnore() below.

The isRunIgnore is not called everywhere outside the class, so adding it is not useful. In fact, it shouldn't have been added if the code was test-driven....

Also the member varialble is never used and can be moved down in the class hierarchy. I recommend to do that as it isn't needed to have that state in the UtestShell.

I wish we could move the setRunIgnored also out of the UTestShell interface, but I wouldn't know how. The reason is that usually this kind of information is given at object construction time, but the UtestShell is constructed at program start, so that isn't possible. So, I guess we'll need to keep that one method in the UtestShell interface.

yeah I agree.
UtestShell::isRunIgnore() is not called outside the class, I will remove it.
For the setRunIgnored and that member variable, I also think defined in UtestShell is not the best design, but due to current class hierarchy(UtestShell and IgnoreTestShell) and inter-class dependency(TestRegistry and UtestShell Interface), we can not get bettern place to hold the setRunIgnored and the member variable, unless we do some refactorying on the class hierrachy, eg. segregate the UtestShell interface. But currently it's not deserved to refactorying the interface. We can do this as the interface beome fatter.

Can you make setRunIgnored virtual. Let the UtestShell implement return true and move the member variable in the subclass.

You do not need the member variable in the base class and removing that makes it simpler.

yeah, I also plan to do that. and what's your thinking of interface segregate for UtestShell? maybe We need to do it in case UtestShell interface become fatter.

Yeah, I don't like the setIgnoreTest in the interface, but for now I think it is ok.

As I mentioned, usually this kind of info is passed on creation, but in our case we cannot do that because the creation happens in the macro. So, for now its ok.

arstrube · 2016-03-23T08:26:03Z

I was just wondering what the output will be of an ignored test that is run.

Currently, the output would be"

.!                             # normal test followed by ignored test
TEST(group, test1) - 0ms       # normal test with -v
IGNORE_TEST(group, test2) -0ms # ignored test with -v

My expectation is that this, when ignored tests are run, changes to:

..                       # normal test followed by ignored test that is run
TEST(group, test1) - 0ms # normal test with -v
TEST(group, test2) - 0ms # ignored test that is run with -v

Is this actually the case? Does everyone agree that this is how it should be? In my opinion, one should be able to tell from the output at a glance whether an ignored test was run or not.

Also, statistics in the summary line should change accordingly, because the ignored tests which are run are no longer ignored. Ignored tests which are not run should stay as is.

arstrube · 2016-03-23T08:32:47Z

I see there is a test for the summary counts. But there isn't one for the output.

cuitoldfish · 2016-03-23T09:06:20Z

i prefer

..                       # normal test followed by ignored test
TEST(group, test1)        # normal test with -v
IGNORE_TEST(group, test2) # ignored test with -v

because user know he is running the IGNORE_TEST, the output should tell this truth.

The statistics has been changed accordingly, when IGNORE_TEST run, ignore count will not increase but the run count will increase

arstrube · 2016-03-23T09:12:49Z

@cuitoldfish hmmm. I think replacing ! by . but not replacing the -v output is inconsistent (if that's really what you meant)

arstrube · 2016-03-23T09:15:45Z

Another, rather small thing -- grammatically speaking, I would prefer runIgnoredSomething in the code to runIgnoreSomethin`.

arstrube · 2016-03-23T14:15:06Z

@basvodde There is no need to change input, as can be seen here:

$ ./CppUTestTests.exe -v "IGNORE_TEST(UtestShell, IgnoreTestAccessingFixture)"
IGNORE_TEST(UtestShell, IgnoreTestAccessingFixture) - 0 ms

OK (613 tests, 0 ran, 0 checks, 1 ignored, 612 filtered out, 0 ms)

$ ./CppUTestTests.exe -v "TEST(UtestShell, IgnoreTestAccessingFixture)"
IGNORE_TEST(UtestShell, IgnoreTestAccessingFixture) - 0 ms

OK (613 tests, 0 ran, 0 checks, 1 ignored, 612 filtered out, 0 ms)

Adding the -ri flag would run the test. No difference there.

cuitoldfish · 2016-03-23T15:19:58Z

@arstrube if I have a ignore test

IGNORE_TEST(group, test2)

then if I run cpputest using this command ./CppUTestTests.exe -v "IGNORE_TEST(UtestShell, IgnoreTestAccessingFixture)"

your expectation is

TEST(group, test2) - 0 ms
OK (613 tests, 1 ran, 0 checks, 0 ignored, 612 filtered out, 0 ms)

seems strange incongruity :)

arstrube · 2016-03-23T15:51:26Z

Yeah, but this "feature" is not part of the requirements that we agreed on :P The requirements say that only if you do
./CppUTestTests.exe -ri -v "IGNORE_TEST(UtestShell, IgnoreTestAccessingFixture)" you can expect

TEST(group, test2) - 0 ms
OK (613 tests, 1 ran, 0 checks, 0 ignored, 612 filtered out, 0 ms)

arstrube · 2016-03-23T15:53:12Z

It may not be what a user might expect, but it is consistent with how we said this thing should work :). I mean, if I specify the same test case by means of -sg / -sn, it isn't going to be run, either.

arstrube · 2016-03-23T15:59:07Z

Did you merge cpputest/master / rebase to cpputest/master? It's still spooking the Dos build ;-)

arstrube · 2016-03-23T16:04:19Z

Oh, it seems like a new problem this time. @basvodde if I recall correctly, TestUTestMacro is rather large also. It may need breaking up; or perhaps @cuitoldfish can reduce the size of his tests by 27 bytes :P....

basvodde · 2016-03-24T01:44:45Z

@arstrube Ok, I'll look at splitting some other time.

What I meant with the output is that is you change it to IGNORE_TEST_RUN_AWAY(gorup, name) then you also need to add that as possible input.

cuitoldfish · 2016-03-24T03:37:12Z

@arstrube , I haven't rebase or merge master, I would like to finalize current implementation and then do that to pass the CI. But did you mention currently there is still issue in TestUTestMacro to hamper the CI?

coveralls · 2016-03-24T03:49:26Z

Coverage decreased (-0.02%) to 99.889% when pulling 9a9d9de on cuitoldfish:runIgnoreTest into ea21a28 on cpputest:master.

coveralls · 2016-03-24T04:14:03Z

Coverage decreased (-0.02%) to 99.889% when pulling 9a9d9de on cuitoldfish:runIgnoreTest into ea21a28 on cpputest:master.

arstrube · 2016-03-24T04:26:36Z

@cuitoldfish maybe better if you merge. If the Dos problem goes away, we're good. @basvodde If not, this won't pass before the UtestMacroTests are split in some way.

arstrube · 2016-03-24T04:29:10Z

@cuitoldfish why, you removed a method didn't you. But the code grew fatter and coverage went down :-)

cuitoldfish · 2016-03-24T04:30:30Z

I removed IgnoredUtestShell::isRunIgnore() because it's not needed as discussed

cuitoldfish · 2016-03-24T04:34:18Z

The decrease may because the virtual method void UtestShell::setRunIgnore(). This is just a interface, it's body will not be executed so that coverage decrease.

arstrube · 2016-03-24T04:45:19Z

You'll need to test it. So it doesn't do anything fishy :-P. Or at least enclose it in

// LCOV_EXCL_START
// LCOV_EXCL_STOP

Of course you had to remove IgnoredUtestShell::isRunIgnore(). I was just joking about removing a method and still the code size in Dos grew by some 1.8 kbytes :)

And, may I point out that the class is IgnoredUtestShell and not IgnoreUtestShell ;-). So, personally, I'd like the method and variable names to be consistent with that.

arstrube · 2016-03-24T04:49:17Z

No problem about passing Dos later, but I'd say the 1.8k problem is different to the original one and only splitting up the UtestMacro tests will make it go away. Not necessarily the files, but the test groups. Because one test group has hit the 64k limit.

cuitoldfish · 2016-03-24T05:54:14Z

I haven't notice the extra d in IgnoredUtestShell :), I will add the d consistently.

coveralls · 2016-03-24T06:12:06Z

Coverage increased (+0.0003%) to 99.907% when pulling 738a373 on cuitoldfish:runIgnoreTest into 20e53b7 on cpputest:master.

basvodde · 2016-03-24T06:20:49Z

Excellent, thanks!

arstrube · 2016-03-24T07:05:01Z

@cuitoldfish nice :)
@basvodde now we need to to split up TestUTestMacro so the Dos build won't fail...

arstrube · 2016-03-24T07:09:46Z

Actually, I'm not entirely clear what the linker means complaining about TestUTestMacro_TEXT2. Looks like it corresponds to the file name, not the test group. So we may have to split the file after all :-(.

cuitoldfish · 2016-03-24T07:18:37Z

I think at least from test design point of view, there should be separate test file for UtestShell and IgnoreTestShell, right?

arstrube · 2016-03-24T07:29:54Z

Sounds good to me.

arstrube · 2016-03-24T08:11:51Z

After looking at it more closely, there were some IgnoredUtestShell tests in there already that conceptionally belong to UtestTest.cpp. Along with your new tests, I will move them there. It is a rather small file. So we don't have to go to the trouble to split files at this point.

liu nick added 4 commits March 20, 2016 04:59

argument line option to enable OPTRUN_TEST

5356ff5

add command line option to enable run ignore test

d3cf3e0

delete extra ;

f71c240

add ; for group test

a5de964

basvodde reviewed Mar 21, 2016
View reviewed changes

change -runIgnore to -ri

bbf318b

basvodde reviewed Mar 22, 2016
View reviewed changes

cuitoldfish reviewed Mar 23, 2016
View reviewed changes

liu nick added 2 commits March 24, 2016 03:24

refactory and formatt output

4933425

formatting correct

9a9d9de

liu nick added 2 commits March 24, 2016 05:38

add test and change ignore to ignored

e974f11

Merge remote-tracking branch 'upstream/master' into OPTIN_TEST

738a373

basvodde merged commit 6700c8f into cpputest:master Mar 24, 2016

cuitoldfish deleted the runIgnoreTest branch March 24, 2016 10:17

		@@ -50,6 +50,7 @@ void TestRegistry::runAllTests(TestResult& result)
		result.testsStarted();
		for (UtestShell *test = tests_; test != NULL; test = test->getNext()) {

add command line option to Run ignore test #908

add command line option to Run ignore test #908

Conversation

cuitoldfish commented Mar 20, 2016

coveralls commented Mar 21, 2016

Choose a reason for hiding this comment

basvodde commented Mar 21, 2016

arstrube commented Mar 21, 2016

basvodde commented Mar 21, 2016

cuitoldfish commented Mar 21, 2016

arstrube commented Mar 21, 2016

cuitoldfish commented Mar 21, 2016

basvodde commented Mar 21, 2016

arstrube commented Mar 21, 2016

cuitoldfish commented Mar 21, 2016

basvodde commented Mar 22, 2016

basvodde commented Mar 22, 2016

cuitoldfish commented Mar 22, 2016

coveralls commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basvodde commented Mar 22, 2016

cuitoldfish commented Mar 22, 2016

basvodde commented Mar 22, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

cuitoldfish commented Mar 23, 2016

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

cuitoldfish commented Mar 23, 2016

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

arstrube commented Mar 23, 2016

basvodde commented Mar 24, 2016

cuitoldfish commented Mar 24, 2016

coveralls commented Mar 24, 2016

coveralls commented Mar 24, 2016

arstrube commented Mar 24, 2016

arstrube commented Mar 24, 2016

cuitoldfish commented Mar 24, 2016

cuitoldfish commented Mar 24, 2016

arstrube commented Mar 24, 2016

arstrube commented Mar 24, 2016

cuitoldfish commented Mar 24, 2016

coveralls commented Mar 24, 2016

basvodde commented Mar 24, 2016

arstrube commented Mar 24, 2016

arstrube commented Mar 24, 2016

cuitoldfish commented Mar 24, 2016

arstrube commented Mar 24, 2016

arstrube commented Mar 24, 2016