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

issue #91: SocketException: Software caused connection abort #171

Merged
merged 2 commits into from
Oct 16, 2015
Merged

issue #91: SocketException: Software caused connection abort #171

merged 2 commits into from
Oct 16, 2015

Conversation

missedone
Copy link
Collaborator

the solution is as i stated in testng-team/testng#777 that use the plugin embedded testing-remote.jar as the agent for communicating TestNG runtime with plugin

the testng-remote source code is splitter out from the main testng git repo: https://github.com/missedone/testng-remote

@@ -329,21 +329,25 @@ protected VMRunnerConfiguration createVMRunner(final ILaunchConfiguration config
public String[] getClasspath(ILaunchConfiguration configuration)
throws CoreException {
String[] originalClasspath = super.getClasspath(configuration);
// add remote testng jar file at the very begining to make sure this is loaded in prior of the user's testng.jar
String[] classpathArray = new String[originalClasspath.length + 1];
classpathArray[0] = BuildPathSupport.getRemoteTestNGLibPath();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add remote testng jar file at the very begining to make sure this is loaded in prior of the user's testng.jar

@missedone
Copy link
Collaborator Author

Hi @cbeust @juherr
with this solution, I had some tests against different version of testng, it turns out that current testng-remote is compatible with 6.5.1 or above, but failed with 6.3.1 or below.

i think what @juherr proposed might solve this. but i'm not sure if we really want to support that old version, or shall we add minimum supported testng version in plugin?

@juherr
Copy link
Member

juherr commented Oct 1, 2015

I suppose 6.5.1 will be enough but more could be fun.
The question is: is it possible to find easily the testng version from classpath?

@missedone
Copy link
Collaborator Author

ah, good question, i'm afraid we can't, org.testng.internal.Version.VERSION only came since 6.8.1 and not really synced with the release version.

@JnRouvignac
Copy link
Contributor

I think it is possible to configure maven to output a MANIFEST.MF file with all the build information. Including when using the mvn release plugin.

@juherr
Copy link
Member

juherr commented Oct 2, 2015

Yep, good idea. I checked and the Implementation-Version is available since 5.0.
And we can use Main-Class: org.testng.TestNG to find the appropriate MANIFEST.MF in classpath.

@missedone
Copy link
Collaborator Author

quoted from @juherr

And why not having a module by API version (in order to have the appropriate testng dependency and have compile time check), load the appropriate facade by reflexion depending on the version and produce an all in one jar with all facade? A bit like what surefire is doing.

generally, i think it's the right approach. But when we're talking about API, it means both the Java Interfaces (e.g. methods signatures) but also the data protocol spec, after look at the version of 6.3.1, my current concern is, 6.3.1 is missing the "instanceName" property in TestResultMessage which means it breaks the data protocol, I'm not sure if this will work or not without "instanceName"...

@missedone
Copy link
Collaborator Author

or, try to make it clear, let's say, the solution need consider at both the plugin side and TestNG runtime side. @juherr, are we both holding the assumption that, there will be only one version of remote testng at plugin side which will communicate with the facade remote testng (which will delegate the request to the specific version of remote testng) at TestNG runtime side?

otherwise, i might have more concerns that let's talk about them later.

@juherr
Copy link
Member

juherr commented Oct 2, 2015

Yes, we are agree.

eclipse -> remote api -> remote impl x -> testng x (maybe x+1, x+2, ... too)
                      -> remote impl y -> testng y (maybe y+1, y+2, .. too)

But it's clearly a "fun" feature with a low priority IMO. We just have to think a bit about it in order to have an architecture which will permit it later.

@missedone
Copy link
Collaborator Author

yes, agreed.

@cbeust
what do you think if this PR, this solution, only support testng 6.5.1 or above? that says the minimum testng version is 6.5.1 since the change.

@cbeust
Copy link
Collaborator

cbeust commented Oct 3, 2015

I'm fine with supporting only 6.5.1+.

@cbeust
Copy link
Collaborator

cbeust commented Oct 3, 2015

Any reason why this PR removes the JUnit 4 jar file?

@missedone
Copy link
Collaborator Author

junit.jar was used by the plugin, now plugin depends on the existing org.junit plugin so that we can save the dist plugin size.
this junit jar is not used at runtime, so it is safe to remove.

@missedone
Copy link
Collaborator Author

@cbeust
how do we proceed this PR?
if it's OK to merge, we will have other thing to think about:

  1. shall we move testng-remote to your official testng github, just like cbeust/testng? so that we all maintain the same git repo
  2. do you think need to remove the remote package out of testng git repo to avoid duplication with testng-remote?

@juherr
Copy link
Member

juherr commented Oct 15, 2015

  1. I propose testng-team group instead :)
  2. +1 to remove duplication once released

@cbeust
Copy link
Collaborator

cbeust commented Oct 15, 2015

My suggestion:

  • Move the testng-remote project to the testng-team org as suggested by @juherr
  • Once this is done, I'll merge this PR

Is everybody okay with this?

@missedone
Copy link
Collaborator Author

yeah, sounds good:)
the new repo is here: https://github.com/testng-team/testng-remote

@cbeust
Copy link
Collaborator

cbeust commented Oct 16, 2015

Thanks, @missedone.

Everybody on board with merging this PR?

@juherr
Copy link
Member

juherr commented Oct 16, 2015

👍

cbeust added a commit that referenced this pull request Oct 16, 2015
issue #91: SocketException: Software caused connection abort
@cbeust cbeust merged commit 2c5873f into testng-team:master Oct 16, 2015
@missedone missedone deleted the bug/91 branch October 16, 2015 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants