Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testng 6.9.12 start to post 3 org.testng.ITestListener#onTestStart events for one test method #1084

Closed
akozlova opened this issue Jul 6, 2016 · 16 comments

Comments

@akozlova
Copy link
Contributor

akozlova commented Jul 6, 2016

Works fine with 6.9.11.

link to the initial issue: https://youtrack.jetbrains.com/issue/IDEA-158214. (IDEA treats each onTestStarted event as new test method and creates a node for it in the tree)

@juherr
Copy link
Member

juherr commented Jul 6, 2016

@akozlova Could you point me where/how you register your listener?
I made some improvments on the API with listeners #1049 and maybe it broke some behaviors (which was not expected).

@akozlova
Copy link
Contributor Author

akozlova commented Jul 6, 2016

Actually the problem is that my listener implements multiple interfaces (ISuiteListener, IResultListener, IInvokedMethodListener) and is registered multiple times as suite listener, etc.

@akozlova
Copy link
Contributor Author

akozlova commented Jul 6, 2016

Yep, I can try to split my listener in multiple classes, should I do that? The main problem with it that the old IDEA versions won't be compatible with new testng :/

@juherr
Copy link
Member

juherr commented Jul 6, 2016

No need to split into multiple classes: just register it only once with addListener(Object) or better with addListener(ITestNGListener) and TestNG will register each interface itself.

Note that only addListener(ITestNGListener) will stay at the end.

But adding the same listener many times is not supposed to call it many times. Will try to reproduce it, or maybe you can propose use a new test case that shows the issue? https://github.com/cbeust/testng/tree/master/src/test/java/test/listeners

@akozlova
Copy link
Contributor Author

akozlova commented Jul 6, 2016

It didn't work with addListener(Object) because it detects the first 'instance of' and do not proceed further (previously that was needed to get the rest of the events). With the last version it works but we need to support previous versions as well. Please note that we can't use addListener(ITestNGListener) for the same compatibility problem.

Looks like whether you replace list with LinkedHashSet or I need to split listeners to different classes.

@juherr
Copy link
Member

juherr commented Jul 6, 2016

Looks like whether you replace list with LinkedHashSet or I need to split listeners to different classes.

Yes, I will try something like that.

Please note that we can't use addListener(ITestNGListener) for the same compatibility problem.

Ok, I take the point. Maybe https://github.com/testng-team/testng-remote/ is a good starting point as it provides some backward compatibilities with previous testng version for the testng-eclipse project.

@akozlova
Copy link
Contributor Author

akozlova commented Jul 6, 2016

That was our previous approach and we had a lot of compatibility issues as the protocol should be the same on both sides, so IDE should bundle exactly the same version user uses to tests. And suppose a user has different testng versions for different tests or has different projects with different versions. That simply doesn't work so we end up with our own protocol on both sides so user always has compatible protocol.

@juherr
Copy link
Member

juherr commented Jul 6, 2016

You should exchange with @missedone because I think it has exactly the same needs and fixes most of them now.

@juherr
Copy link
Member

juherr commented Jul 6, 2016

But don't worry, it is not yet planned to break the API ;)

@missedone
Copy link
Contributor

hi @akozlova

That simply doesn't work so we end up with our own protocol on both sides so user always has compatible protocol.

with the new protocol json of this new testng-remote https://github.com/testng-team/testng-remote/, it suppose to fix the version compatible issue you mentioned.
hope you can have a look and let us know your thought.

anyway, let's start the discussion in a new thread as you like. (don't want to pollute current one 😄 )

@akozlova
Copy link
Contributor Author

akozlova commented Jul 8, 2016

Guys, now we have one common protocol between all tests (junit, testng, javascript, python, etc) and we are quite happy with it. In order to adapt new remote protocol for testng I would have to translate it into our protocol, I don't think that it's feasible.

Please make addListener(Object) not deprecated and IDEA would be happy. Thanks

@juherr
Copy link
Member

juherr commented Jul 8, 2016

@akozlova And what about something like:

    public static void addListener(TestNG tng, Object listener) {
        Method addListener;
        try {
            addListener = tng.getClass().getMethod("addListener", ITestNGListener.class);
        } catch (NoSuchMethodException e) {
            try {
                addListener = tng.getClass().getMethod("addListener", Object.class);
            } catch (NoSuchMethodException e1) {
                throw new IllegalArgumentException("No addListener method found", e1);
            }
        }
        try {
            addListener.invoke(tng, listener);
        } catch (IllegalAccessException | InvocationTargetException e) {
            throw new RuntimeException(e);
        }
    }

@akozlova
Copy link
Contributor Author

akozlova commented Jul 8, 2016

I have a lot of reflection there already, I can add another one, no problem.
Could you please restore the previous behaviour and leave the deprecation for some years?
Otherwise people who use IDEA from year 2013 and gradle with testng dependency 6.9.+ would complain or we have to build, test and upload 4-5 releases.

@juherr
Copy link
Member

juherr commented Jul 8, 2016

Could you please restore the previous behaviour and leave the deprecation for some years?

Sure, I will.

@cbeust
Copy link
Collaborator

cbeust commented Jul 8, 2016

@juherr and please add a comment to that method to make it clear that IDEA needs it.

Thanks!

@cbeust cbeust closed this as completed in 48f69f2 Jul 14, 2016
cbeust added a commit that referenced this issue Jul 14, 2016
@akozlova
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants