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.

Sign up for GitHub

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

Jump to bottom

Add support for Priority in Configuration Methods #2662

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for Priority in Configuration Methods #2662

wants to merge 1 commit into from

Conversation

martinaldrin
Copy link

@martinaldrin martinaldrin commented Oct 25, 2021 •
edited

Fix #2663

@martinaldrin martinaldrin force-pushed the priority branch 4 times, most recently from cd9cbf2 to 5cc17a5 Compare October 25, 2021 18:38
@martinaldrin
Copy link
Author

I got some time to clean up the code and added one simple test.

krmahadevan
krmahadevan requested changes Oct 26, 2021
View reviewed changes
testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java Outdated
public class ConfigurationMethodPriorityTest extends SimpleBaseTest {

@Test(description = "GITHUB-2662")
public void ensureThatPriorityWorksOnConfigratuionMethods() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please help fix the typo in the method name ?

Copy link
Author

Choose a reason for hiding this comment

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

Done

testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java
import org.testng.TestNG;
import org.testng.annotations.Test;

public class ConfigurationMethodPriorityTest extends SimpleBaseTest {
Copy link
Member

Choose a reason for hiding this comment

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

Can we please add tests for the following combination ?

  1. Multiple @BeforeXX methods with different priorities across multiple classes (Multi level inheritance)
  2. Multiple @BeforeXX methods with different priorities across multiple classes (No inheritance, but the configuration method just resides in a different class. This would be true in the case of @BeforeTest and @BeforeSuite and perhaps even @BeforeGroups as well)
  3. Multiple @BeforeXX methods with same priorities in same class
  4. Multiple @BeforeXX methods with same priorities in different class (Multi level inheritance)
  5. Multiple @BeforeXX methods with same priorities in different class (No multi level inheritance)
    (3), (4), (5) - is to check if the logic defined here is getting utilised properly
  6. Multiple @BeforeSuite methods with different priorities defined across multiple <suite> tags.
  7. Multiple @BeforeSuite methods with same priorities defined across multiple <suite> tags.
  8. Multiple @BeforeSuite methods with different priorities defined across multiple <suite> tags and with parallel attribute set to <test>

Copy link
Author

Choose a reason for hiding this comment

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

I will try to find the test for Test Annotation and implement in a similar way

testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java
import org.testng.TestNG;
import org.testng.annotations.Test;

public class ConfigurationMethodPriorityTest extends SimpleBaseTest {
Copy link
Member

Choose a reason for hiding this comment

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

Also please add a reference of this class under src/test/resources/testng.xml so that it gets executed as part of the test execution.

Also please house this class, the test class sample that this test uses and all new test samples that we would be needing, under the package test.configuration

Copy link
Author

Choose a reason for hiding this comment

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

Done

CHANGES.txt Outdated
@@ -1,4 +1,5 @@
Current
New: GITHUB-2662 Add support for Priority in Configuration Methods.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please help create a new GITHUB issue and add details to that issue and have this updated to refer to that issue ? That way it would be easier to track. Also please add reference to your name ( We dont want to miss giving credit for this delivery to you)

Copy link
Author

Choose a reason for hiding this comment

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

Done

@martinaldrin
Copy link
Author

Many thanks I will try to fix these comments during the day?
For all these unit test, do you have similar test for Test Annotation? so I can follow the same structure of building my new tests?

@martinaldrin
Copy link
Author

Can not write unit test due to new issue found
#2664

@martinaldrin martinaldrin force-pushed the priority branch 2 times, most recently from d4be735 to d3346b7 Compare October 27, 2021 12:58
juherr
juherr requested changes Oct 28, 2021
View reviewed changes
testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java
"beforeMethodA",
"TestB",
"afterMethodB",
"afterMethodA",
Copy link
Member

Choose a reason for hiding this comment

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

afterMethodA is not supposed to be run as TestA was not run yet.

testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java
"TestB",
"afterMethodB",
"afterMethodA",
"beforeMethodB",
Copy link
Member

Choose a reason for hiding this comment

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

beforeMethodB and beforeMethodA are not supposed to be run as they were already run previously.

testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java Outdated
public void ensureThatPriorityWorksOnConfigurationMethodsWithMethodDependency() {
List<String> expectedOrder1 =
Arrays.asList(
"s3", "s2", "s1", "t3", "t2", "t1", "c3", "c2", "c1", "m3", "m2", "m1", "test3", "m3",
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename methods in order to make them easy to read in the assertion?

testng-core/src/test/java/test/ConfigurationMethodPriorityTest.java Outdated
tng.addListener(listener);
tng.run();
System.out.println(expectedOrder1);
System.out.println(listener.getSucceedMethodNames());
Copy link
Member

Choose a reason for hiding this comment

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

Please remove syso :)

@martinaldrin
Copy link
Author

I will do the cleanup of comments and add more unit tests when #2664 is sorted out. since it is blocking this PR.

@martinaldrin martinaldrin mentioned this pull request Nov 2, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juherr juherr juherr requested changes

@krmahadevan krmahadevan krmahadevan requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Priority in Configuration Methods
3 participants
@martinaldrin @juherr @krmahadevan