Skip to content

Duplicate names fix#1526

Merged
jdneo merged 11 commits intomicrosoft:mainfrom
Kropie:duplicate_names_fix
Feb 9, 2023
Merged

Duplicate names fix#1526
jdneo merged 11 commits intomicrosoft:mainfrom
Kropie:duplicate_names_fix

Conversation

@Kropie
Copy link
Copy Markdown
Contributor

@Kropie Kropie commented Feb 5, 2023

Fixes #1517

Solution Description

When creating test IDs for TestNG and JUnit5 tests, the method parameters are included in the test IDs. This allows for multiple tests of the same name to be correctly identified on the server and the client.

@jdneo jdneo self-requested a review February 6, 2023 02:02
target's outFiles array in the "launch.json" file to match the
"Launch Tests (maven-junit)" target
Copy link
Copy Markdown
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

Just some small pieces.

BTW, I noticed that you deleted the changes in junit5/AppTest. I suggest keeping that. Although we covered it in the test cases but imagine if the output format of the test runner changes, we can still leverage it to do some quick manual verification.

@jdneo jdneo added this to the 0.38.0 milestone Feb 7, 2023
@Kropie
Copy link
Copy Markdown
Contributor Author

Kropie commented Feb 7, 2023

Just some small pieces.

BTW, I noticed that you deleted the changes in junit5/AppTest. I suggest keeping that. Although we covered it in the test cases but imagine if the output format of the test runner changes, we can still leverage it to do some quick manual verification.

I'll go ahead and bring the changes back then

@jdneo
Copy link
Copy Markdown
Member

jdneo commented Feb 7, 2023

I tested the change for TestNG project. looks like there is some problem parsing the test suite:

See:

Now the method name will contain the parameter types, but I found that TestNG has some problem if the method name contains parameter types. (It cannot find the test method in this case)

One quick workaround can change the return statement to return clazz.substring(clazz.lastIndexOf("#") + 1, clazz.indexOf("("));, though it will trigger all the test methods with the same name. But that looks like a problem of TestNG itself.

BTW, the argument name clazz is misleading. It's test id actually.

You can use the following test class to verify:

import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

public class CharUtilsTest {

	@DataProvider
	public Object[][] ValidDataProvider() {
		return new Object[][]{
				{ 'A', 65 },{ 'a', 97 },
				{ 'B', 66 },{ 'b', 98 },
				{ 'C', 67 },{ 'c', 99 },
				{ 'D', 68 },{ 'd', 100 },
				{ 'Z', 90 },{ 'z', 122 },
				{ '1', 49 },{ '9', 517 }
 
		};
	}

	@Test(dataProvider = "ValidDataProvider")
	public void CharToASCIITest(final char character, final int ascii) {
		int result = CharUtils.CharToASCII(character); 
		Assert.assertEquals(result, ascii);
	}

	 @Test
	 public void CharToASCIITest() {
	 	Assert.assertEquals(1, 2);
 	 }
}

@Kropie
Copy link
Copy Markdown
Contributor Author

Kropie commented Feb 8, 2023

I tested the change for TestNG project. looks like there is some problem parsing the test suite:

See:

Now the method name will contain the parameter types, but I found that TestNG has some problem if the method name contains parameter types. (It cannot find the test method in this case)

One quick workaround can change the return statement to return clazz.substring(clazz.lastIndexOf("#") + 1, clazz.indexOf("("));, though it will trigger all the test methods with the same name. But that looks like a problem of TestNG itself.

BTW, the argument name clazz is misleading. It's test id actually.

You can use the following test class to verify:

import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

public class CharUtilsTest {

	@DataProvider
	public Object[][] ValidDataProvider() {
		return new Object[][]{
				{ 'A', 65 },{ 'a', 97 },
				{ 'B', 66 },{ 'b', 98 },
				{ 'C', 67 },{ 'c', 99 },
				{ 'D', 68 },{ 'd', 100 },
				{ 'Z', 90 },{ 'z', 122 },
				{ '1', 49 },{ '9', 517 }
 
		};
	}

	@Test(dataProvider = "ValidDataProvider")
	public void CharToASCIITest(final char character, final int ascii) {
		int result = CharUtils.CharToASCII(character); 
		Assert.assertEquals(result, ascii);
	}

	 @Test
	 public void CharToASCIITest() {
	 	Assert.assertEquals(1, 2);
 	 }
}

Just made some updates to resolve this issue with testng tests.

Copy link
Copy Markdown
Member

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

Thank you @Kropie! All the changes look good to me.

This is a complex one, but you get it done so so well. 🎉

@jdneo jdneo merged commit d46a592 into microsoft:main Feb 9, 2023
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.

Bug: Overload test methods are not handled properly

2 participants