testng-team / testng

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

Incorrect listener's onStart(ISuite suite) execution order in case of inner suites #1656

Open avarabyeu opened 6 years ago

avarabyeu commented 6 years ago

TestNG Version

Note: only the latest version is supported

6.11

Expected behavior

The listener is called for each suite, if the parent suite contains child suites then the parent suites should run first before running the child suite.

Actual behavior

The child suites are first run before running the parent suite.

Is the issue reproducible on runner?

Test case sample

<!DOCTYPE suite SYSTEM "http://testng.org/testng-1.0.dtd">
<suite name="ParentSuite" parallel="classes" thread-count="10">

    <suite-files>
        <suite-file path="child_suite.xml"/>
    </suite-files>

    <test verbose="1" name="SomeTest">
        <classes>
            <class name="com.example.SomeTest"/>
        </classes>
    </test>

</suite>
juherr commented 6 years ago

Currently, suite-files is like an import, include <test> from specified suites and the result is only one suite. That's why onStart is only called once.

avarabyeu commented 6 years ago

@juherr actually it's called twice (for each suite file). Has the behaviour changed since 6.11?

juherr commented 6 years ago

Ok, two calls is an issue.

@krmahadevan What is the expected behavior of onStart in case of suite-files?

krmahadevan commented 6 years ago

@juherr - I think that's the correct behavior.

As part of #1285 I think we changed this behavior

juherr commented 6 years ago

I am able to reproduce the order issue but, in fact, TestNG was always running child suites before parent suite (https://github.com/cbeust/testng/blame/master/src/main/java/org/testng/TestNG.java#L1191-L1199) and listeners were not correctly added before #1285.

juherr commented 6 years ago

6.13 should have fixed the duplicate issue with #1533

juherr commented 6 years ago

@avarabyeu I can change the order behavior and run the master suite before the child suites but the master suite will finish before its children. Will it be ok for you? A better child management implies big changes we will probably don't do.

Could you confirm that 6.13 is fixing the event duplication issue?

avarabyeu commented 6 years ago

@juherr Can confirm that 6.13 does not have issue with duplication.

I can change the order behavior and run the master suite before the child suites but the master suite will finish before its children.

Yeah it would be easier for us to handle events in this case. Thank you!

juherr commented 6 years ago

@cbeust @krmahadevan WDYT if we change the order execution of suite/child?

krmahadevan commented 6 years ago

@avarabyeu - Just trying to understand, how does the order of execution matter ? Why is it problematic if the child suites execute first and then the parent suite ?

@juherr - do we really need to change this ? We can easily have this sorted out at the test level itself by moving the contents of the master suite into a suite of its own, and then having the master suite just work as a suite of suites no ? Just trying to understand what we are going to get as benefits ?

juherr commented 6 years ago

@krmahadevan You will find more details on https://github.com/reportportal/reportportal/issues/273 The goal is to find a short fix for the issue without breaking things. With more time, I'd prefer to have proper child suites management but I'm not feeling it very important. If I understand your solution, you propose to merge all child <test> into the parent <suite>, right?

avarabyeu commented 6 years ago

@krmahadevan ReportPortal is a tool for real-time reporting of results from different test engines. So basically, its client handles events fired by testng listener and sends them to server. Order of that events matter because based on them ReportPortal fills internal data structures which are tree-based - for instance, it's impossible to create child before the parent.

krmahadevan commented 6 years ago

@juherr

If I understand your solution, you propose to merge all child into the parent , right?

No. That is not what I meant. I am basically saying that the parent suite xml file should not have any <test> tag entries in it but only contain <suite-file> entries.

krmahadevan commented 6 years ago

@avarabyeu - Thanks for sharing additional context. I am still not able to grasp the idea behind the order of execution being important, because TestNG does fire events before and after suite execution. So irrespective of whether child suites execute or the parent, the corresponding events get triggered no ?

juherr commented 6 years ago

No. That is not what I meant. I am basically saying that the parent suite xml file should not have any tag entries in it but only contain entries.

Ok! True, it is something we can check and warn against.

avarabyeu commented 6 years ago

So irrespective of whether child suites execute or the parent, the corresponding events get triggered no ?

Yeah, but we build the same structure in our internal database - parent suite -> child suite. So if child suite starts earlier then parent, we simply cannot handle it on server side.

krmahadevan commented 6 years ago

@avarabyeu - So what happens if you don't have any <test> tags at all in your master suite file but it has only <suite-file> tags in it ? Would that not work for you ?

I am basically trying to see what can be done to help you get past the problem without any code change.

avarabyeu commented 6 years ago

@krmahadevan actually, this is good question. Issue came from our clients and i'm not really sure if they are able to adjust their testng.xml files in order to solve this integration issue. Let me check what happens if we have no in master suite.

krmahadevan commented 6 years ago

Closing this issue for now. @avarabyeu Please comment back if you have an update on the query that I raised so that we can further triage this issue if necessary.

ulmanA commented 6 years ago

@krmahadevan @avarabyeu I tried to execute suite of suite-files, without any <test> tags in the master suite file and the problem still happened. (tested on TestNG v6.14.3)

krmahadevan commented 6 years ago

There were some changes made to the latest codebase in TestNG with respect to ordering of suites etc.,

So can you please try once again using TestNG 7.0.0-SNAPSHOT and post back your results ?

If its still a problem, request you to please help share a sample that can be used to reproduce the problem.

ulmanA commented 6 years ago

The same problem happened on version 7.0.0-SNAPSHOT attaching example maven project testng-reportportal-example.zip

krmahadevan commented 6 years ago

@ulmanA - The sample you shared doesn't say a lot of things (at-least it wasn't explicit for me).

You basically have a suite file called all-suites.xml which looks like below

<suite name="All Suites" verbose="2">
    <suite-files>
        <suite-file path="suite1.xml"/>
        <suite-file path="suite2.xml"/>
    </suite-files>
</suite>

And when I run this, I notice that suite1 gets executed first, then suite2, leading to all_suites completing itself.

...
... TestNG 7.0.0-SNAPSHOT by Cédric Beust (cedric@beust.com)
...
Suite 1 -> TEST 1
Suite 1 -> TEST 2

===============================================
All Suites (0)
Total tests run: 2, Passes: 2, Failures: 0, Skips: 0
===============================================
Suite 2 -> TEST 2
Suite 2 -> TEST 1

===============================================
All Suites (1)
Total tests run: 2, Passes: 2, Failures: 0, Skips: 0
===============================================

===============================================
All Suites
Total tests run: 4, Passes: 4, Failures: 0, Skips: 0
===============================================

So what is your expectation here ? Its not clear.

all_suites is visualized as a container of suites so its going to be finishing at the last.

ulmanA commented 6 years ago

@avarabyeu Please answer.

nebehr commented 6 years ago

@krmahadevan I am not the bug reporter but I would expect the following order of execution:

  1. ISuiteListener#onStart - all_suites
  2. ISuiteListener#onStart - suite1
  3. ISuiteListener#onFinish - suite1
  4. ISuiteListener#onStart - suite2
  5. ISuiteListener#onFinish - suite2
  6. ISuiteListener#onFinish - all_suites

So yes, all_suites should finish last but it should start first, because it contains all other suites! However, currently all_suites behaves not as a root container but as a container of any items not included in the nested suites.