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

Restore Ordering for DependsOnGroups by using separate Priority on Configuration Methods alternative 2 #2668

Closed

Conversation

martinaldrin
Copy link

Add support for Priority in Configuration Methods and restore DependsOnGroups alternativ 2

Fixes # .

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt

We encourage pull requests that:

  • Add new features to TestNG (or)
  • Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

Restore Ordering for DependsOnGroups.
@martinaldrin
Copy link
Author

The only you need to focus on when review this PR is the ClassHierarchyPriority which replace the dependsOnUpon

@martinaldrin martinaldrin changed the title Add support for Priority in Configuration Methods and restore DependsOnGroups alternativ 2 Restore Ordering for DependsOnGroups by using separate Priority on Configuration Methods alternative 2 Nov 2, 2021
@juherr juherr linked an issue Nov 2, 2021 that may be closed by this pull request
@Elisedlund-ericsson
Copy link
Contributor

I personally prefer this alternative 2 over alternative 1 which reused the regular priority which could possibly cause issues or strange sorting for someone using the regular priority.

this alternative 2 make it very clear how it sorting

@martinaldrin
Copy link
Author

@Elisedlund-ericsson thanks Elis, If more agree with that I will try to separate the Restore of Group sorting into a different PullRequest to separate it from the priority implementation.

Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

I don't get the link between classHierarchyPriority and priority.
Where is the classHierarchyPriority set?

@martinaldrin
Copy link
Author

martinaldrin commented Nov 2, 2021

@juherr classHierarchyPriority is only set in MethodInheritance
regular priority is configured by the user in the annotation.

                    m2.addMethodDependedUpon(MethodHelper.calculateMethodCanonicalName(m1));
                    if (m1.getClassHierarchyPriority() >= m2.getClassHierarchyPriority()) {
                      m2.setClassHierarchyPriority(m2.getClassHierarchyPriority() + 1);
                    }

Both of them are later used in the comparators to sort the methods in correct order.

I got the Idea about a new internal priority when I saw how "InterceptedPriority" was implemented and only used internally. Therefor I thought that this could work similar in a similar way

@juherr
Copy link
Member

juherr commented Nov 2, 2021

@martinaldrin Ok, I see 2 different subjects, right? Is it possible to make 2 different PR?

@juherr
Copy link
Member

juherr commented Nov 2, 2021

btw, I like the classHierarchyPriority idea 👍

Do you see a way to exclude it from the public API? Same remark for InterceptedPriority.

@martinaldrin
Copy link
Author

@juherr, yes

great that you liked classHierarchyPriority!

I have following PRs that I plan to work more with:
[GITHUB-2664] Restore DependsOnGroups Behaviour. #2666
Add support for Priority in Configuration Methods #2662

#2667 and #2668 was just to show the proof of concept that it was possible to solve. So I will cancel these two and continue with the other two.

@martinaldrin
Copy link
Author

I close this PR since it was just a Proof of Concept, will continue in #2666

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.

Add support for Priority in Configuration Methods
3 participants