testng-team / testng

TestNG testing framework
https://testng.org
Apache License 2.0
1.98k stars 1.02k forks source link

A new extension of SkipException to support retry check when exception happened #2107

Closed eyehwan closed 5 years ago

eyehwan commented 5 years ago

TestNG Version

Latest 7.0.0 SNAPSHOT

Expected behavior

Add a new extension of SkipException, to let it support to retry when skip exception happened, when met following conditions:

  1. There is a field to control retry check turned on/off when skip exception happened.
  2. Retry with skip only support at test method, not configuration method.
  3. Implementation of IRetryAnalyzer is added for this test method.

Test case sample

will come up with a pull request.

krmahadevan commented 5 years ago

@eyehwan - I am not quite sure I understand what exactly are we trying to solve using this PR #2108 Can you please help elaborate a bit more ?

eyehwan commented 5 years ago

Yes, of course. Current retry machanism support to trigger retry only when assertion failed or runtime exception happened. But there are some scenarios, when skip exception happened, it is also meaningful to trigger the retry. That's why I provide a new type of skip exception here, when such skip exception happened, depending on whether we have IRetryAnalyzer implementations, we would decide if retry is needed.

krmahadevan commented 5 years ago

A SkipException is specifically designed for skipping a test. Why would someone throw that when they want the test to be retried ? I am not sure I quite understand the use case.

eyehwan commented 5 years ago

Because the user who was writting test and the user who is using/running test may be different, they have different purpose to design and run the test, also on the skipException whether need to trigger retry.

Such case usually happened at some integration test of "complicated" products, for the same test, in the different level of integrations, skip is given different meanings.

I want to make skip exception to be flexible to suit such requirement.

krmahadevan commented 5 years ago

Because the user who was writting test and the user who is using/running test may be different, they have different purpose to design and run the test, also on the skipException whether need to trigger retry.

What does this mean? Can you please elaborate ? From what i understand, you seem to be trying to Fix a design problem in your test framework, by putting a changeset in TestNG.

Such case usually happened at some integration test of "complicated" products, for the same test, in the different level of integrations, skip is given different meanings.

Why should exceptions have different meanings? Exceptions were never meant to be contextual. They were always meant to represent the exact same problem no?

krmahadevan commented 5 years ago

Moreover, the changes look like they could very well reside within a test project and don't necessarily need to be part of TestNG right ?

eyehwan commented 5 years ago

Exceptions were never meant to be contextual.

I disagree with this point of view. The purpose of exceptions would be changed in different contexts.

The purpose of this pull request just want skip exception to be negotiable with outside.

Moreover, I also believe that is why "isSkip" exists as when SkipException created.

krmahadevan commented 5 years ago

I disagree with this point of view. The purpose of exceptions would be changed in different contexts.

Do you want to cite out examples of this claim? I would be curious to know.

The purpose of this pull request just want skip exception to be negotiable with outside.

That should still be possible from outside via a TestNG listener no? Why would you want this to reside inside TestNG. This is what I am trying to understand.

Moreover, I also believe that is why "isSkip" exists as when SkipException created.

I wouldn't know the history context. @juherr What are your thoughts on this PR ?

eyehwan commented 5 years ago

Do you want to cite out examples of this claim? I would be curious to know. That should still be possible from outside via a TestNG listener no? Why would you want this to reside inside TestNG. This is what I am trying to understand.

@krmahadevan, yes, of course I can put it outside TestNG, actually I already do so and be using it for a while. The reason I recommend adding it inside TestNG is about benifits would be brought into TestNG: Retry machanizm in TestNG could be enhanced by adding skip case. Current retry in TestNG only support failed status.

Thinking about this case: After a test was designed and created, it is being used in the different levels of integration test. A "SkipExcpetion" as part of logic in the test usually means one condition can not be met to execute the rest of test. For example one device is not stable occasionally, but the role of this device in the test could be changed in different test purposes or integration levels. It could be a key node or only an assistant node for the same test. When it is just "an assitant node", If we can trigger retry machanism by adding extra recovery actions, device become stable, the test would be executed. Otherwise the whole test session together with other tests should be re-executed, This is very time consuming for a large integration system, especially for an unstable device which is just an assistant tool, not the key part of test logic.

cjayswal commented 5 years ago

I Do agree with @krmahadevan A SkipException is specifically designed for skipping a test. Why would someone throw that when they want the test to be retried. Instead of throwing skip exception you should throw other meaningful custom exception.

Regarding the case you are considering, your framework/implementation can take care it by throwing meaningful custom exception that you can consider in retry analyzer or implementing appropriate listener. For example you can handle driver creation exception and throw custom exception. Another example is QAF provides driver listener and element listener where you can do the needful.

krmahadevan commented 5 years ago

@eyehwan

The reason I recommend adding it inside TestNG is about benifits would be brought into TestNG: Retry machanizm in TestNG could be enhanced by adding skip case. Current retry in TestNG only support failed status.

Sounds like a very good use case to live as a separate project. I think you should publish this as a separate library outside of TestNG because going by what you are saying its something like a decorator that enriches the current functionality. We would like to keep TestNG to enable such things and not necessarily absorb all possible customizations tweaks etc., that can be done to TestNG. Doing that would cause TestNG to bloat and we end up providing things that aren't necessarily needed by everyone.

eyehwan commented 5 years ago

We would like to keep TestNG to enable such things and not necessarily absorb all possible customizations tweaks etc., that can be done to TestNG. Doing that would cause TestNG to bloat and we end up providing things that aren't necessarily needed by everyone.

Okay, I understand. But considerring TestNG is not limited at unit test, but also declare to support many other types of test, I am hoping to get them more involved into TestNG. Let's discuss it next time.

Another example is QAF provides driver listener and element listener where you can do the needful.

@cjayswal , it looks like an good example of @krmahadevan mentioned above about "seperate library". Thanks, I will take a look at it,

juherr commented 5 years ago

Close: should be in a 3rd party lib