ActionManagerTests CrashTest refinment.

[Tosik86]

Describe your changes

• Add descriptive comments to CrashTest code.
• Change timing to more meaningful values.
• Add logging to check timing.
• Fix Grossini sprite removal in CrashTest. Original code was trying to remove Grossini Sprite from itself, not from the scene.

Please check explanation here: #2155

Issue ticket number and link

Checklist before requesting a review

For each PR

• Add Copyright if it missed:
  - "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

• I have performed a self-review of my code.

  Optional:

  • I have checked readme and add important infos to this PR.
  • I have added/adapted some tests too.

For core/new feature PR

• I have checked readme and add important infos to this PR.
• I have added thorough tests.

[rh101]

At the time a crash test is written, that specific code would have resulted in a crash, and the source of that crash would have been fixed. As strange as crash test code may be, it should not be modified, at least not without careful consideration, because you may be changing the logic that initially caused the crash.

Updating things like AXLOG to the new logging defines is fine etc., but anything more than that should not be done, specifically to crash test implementations, no matter how odd the code logic may appear to be.

In this case, I feel that the majority of the modifications you have made to void CrashTest::onEnter() and other related logic changes should be reverted, since it's changed the entire behavior of that crash test.

[aismann]

In this case, I feel that the majority of the modifications you have made to void CrashTest::onEnter() and other related logic changes should be reverted, since it's changed the entire behavior of that crash test.

Is there a reason for a rewrite it's better you let the "old" test as it is and write a "next".

[Tosik86]

Hi!
Tnx for comments. Okey, I got it. This is the part which should not be touched. In this case I even don't see the point to add AXLOGI messages. The reason is that code in this CrashTest is completely bizarre, especially part which tries to remove Grossini Sprite from itself (in reality just nothing happen of course). Then my proposal:

• Just add comment to CrashTest: "Do not touch. This TestCase was implemented in the past to test specific crash scenario." This at least will save people from wasting their time trying to understand what this code is doing :)
• @aismann As far as I understood your proposal is to crate another TestCase, right? I will be happy to do that. So, an idea of my test was to check if AXMOL does not crash if we remove Node while ActionInterval(s) of this Node are being executed.

Does this make sense? If yes, I will do changes in the same PR. Actually I will just reset all my commits and do new ones, then will force push new changes.

[smilediver]

I also think that this PR should be closed, as it doesn't address any issues. But it's awesome that you want to contribute, so I'll just leave a bunch of hopefully helpful random thoughts and tips, that may help you guide your efforts:

• As noted, you need to be careful when modifying actual test code, because it might be setup this way specifically to trigger an issue. What may seem like an "illogical" sequence, might be the actual required steps that reproduced some issue at some point.
• You don't need to add "do not touch", because it's already implied. Or you'll end up adding it to every test. :)
• People don't really care about test code much. Its main purpose is to reproduce an issue and make sure that something is running as intended, so test code is not always perfect, and that's ok. It doesn't serve much purpose to randomly go around changing test code.
• But it might make sense to improve test's code if you're already modifying it for other reasons: like adding a new test case, etc.
• Log messages increase spam in console, and when there's too many of them they become useless and annoying. So before adding logs make sure they actually add any value.
• If you want to test something and it's easily reproducible without starting and using the whole engine, then prefer unit-tests. A lot of tests in cpp-tests are bad, because they are manual and you have no idea what you should be seeing, while unit-tests check specific conditions and they run automatically.

If you're eager to contribute something to the project, then you could try checking already known issues, or maybe search the project for TODO/FIXME comments, or you could create a discussion thread and people could point you to some stuff.

[rh101]

• @aismann As far as I understood your proposal is to crate another TestCase, right? I will be happy to do that. So, an idea of my test was to check if AXMOL does not crash if we remove Node while ActionInterval(s) of this Node are being executed.

Does this make sense? If yes, I will do changes in the same PR. Actually I will just reset all my commits and do new ones, then will force push new changes.

There seems to be some misunderstanding. A crash test case is written to test for a specific bug that would cause a crash, in order to test the resulting fix. So, writing a test "to check if AXMOL does not crash if we remove Node while ActionInterval(s) of this Node are being executed." has no purpose, because there is no bug, unless there is one that has not yet been submitted by you?

[Tosik86]

Hm... okey. I will close PR of course. I just made some investigations out of curiosity and found out that logic of CrashTest actually already was changed many years ago. And probably by mistake? Check PR of original Cocos2d-x repo:cocos2d/cocos2d-x#11272

@smilediver Tnx, I will try to follow your hints.