-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add support for Priority in Configuration Methods #2663
Comments
I'm not sure mixing configuration methods and priority will be a good idea. Is it the expected behavior? |
Hi, I'm not sure if I understand your concern. In my test I don't see that behavior that you mention. The only problem I have is to get my unit test working when I add the test to "testng-core/src/test/resources/testng.xml" /Martin |
From the functional point of view, I don't know what will be the expected behavior because priority is suite scoped, not class scoped. It means all configuration methods will be considered when edges will be created. I think the feature won't be easy to add and need a lot of tests. @krmahadevan thought? |
The priority is not a global feature, e.g. it will sort the method within the same annotation type. E.g. It allows you to sort which BeforeSuite or BeforeMethod should be executed first, today many users solves this by name the so they are sorted in correct order. I don't think this will conflict with any other features either. |
@martinaldrin To be honest, I dont think there are a lot of users who establish dependency amongst configuration methods. In the close to a decade of being part of TestNG and responding to different queries on different forums (Github, LinkedIn, Stackoverflow, Google forums) I have seen people ask about dependencies between tests, but dependency between configurations is something that i have not heard of. Also this solution/alternative of users resorting alphabetising their configurations in order to get the order right is not documented anywhere to the best of my knowledge and I have not seen too many users who dig into the code to that level to find out this information. I second what @juherr said that this feature of adding a priority to configuration methods is not something that is going to be easy to solve. Today TestNG has a notion of method interceptors which are basically invoked by TestNG wherein it allows the testng user to order the tests to whatever order they want. Maybe we can consider building a method interceptor for configurations as well, wherein we just let the testng user tell us what is the order in which they would like to run the test. That way, it needn't be just priorities or alphabetical ordering by names. It could be anything. That functionality would ensure that the customisation or configuration method ordering is externalised with minimal changes. The test method interceptor AFAIK only extracts out independent methods and gives it to the user for ordering. So the same behaviour would be expected for configurations as well. But before all that, we would need to first define what this capability actually means. Are we trying to order the configurations of the same type based on some order? What should the scope be for that. Should it be confined to within a What should the behaviour be when this ordering is coupled with hard dependencies ( |
Hi @krmahadevan , thanks for the reply
We use configuration methods allot, so for us it is important. No we have not digging in to the code to find that out the sorting mechanism, that was just something we figured out. I cloned TestNg for the first time 3 weeks ago, before I have just been a regular user.
Maybe I don't have the full picture, but I got exactly the behavior I expected by adding priority for the configuration methods. Ordering and everything is already handled by TestNg, this was just a feature that I enabled. So no code needs to be changed due to that.
Yes, I know there is other ways to solve this, like intercept the methods in the order we want to run them. But that is much more complicated way of doing it, And most of our users just prefer the standard way using TestNg.
Before Suite is on level, and the other methods are on level, no changes should be made
In my view, everything should work as it does with the dependsOnGroup/Methods, should not be any difference. The only thing that priority will do is to sort the method in a different order compared to the default alphabetic order. Which enable users to control the order in a very easy way with standard api:s. I'm not sure I fully understand your concerns her, since this is just a feature that allows to sort the methods in a different way. Support already exist in TestNg, It will not interfere with dependsOnGroups/Methods, since dependsOn have higher priority than a user defined priority. But I think we first need to focus on restoring #2664 since is a blocker for us for uplifting to TestNg and it is a blocker for this PR as well. |
Sure. Not discounting the fact that this feature is important to you folks. This was more of a general observation based on what I have seen as user questions/issues etc., over the period of time.
What me and @juherr are trying to state here is that, we dont have a historic understanding of what this functionality is supposed to do. So we would need to ensure that while we enable this feature, we also don't break any of the existing tests and to be honest I dont know what all other functionalities/deviations exist with this feature that can break and can surface just like it surfaced with you folks.
To be honest there's no complications. Once we have an interception mechanism for configuration methods, we can basically expose call backs at the following levels (which testng users will consume via listeners)
Now within the listener, you would just need to find out how you would want to order the methods. It could be via a custom configuration such as I personally feel that this is a lesser invasive change into TestNG wherein we aren't adding anything extra, but just exposing the capability of defining the order of configuration methods. The one pitfall with this approach would that a user would be able to order ONLY independent methods and not the ones that have the below attributes:
Since this is a lesser documented feature when compared to all the other functionalities of TestNG, I am just concerned that we may perhaps not have enough tests that will vet out all of the expected behaviours so that at-least now we would have a benchmark of what the functionality works like. I am saying this more from a maintainer's perspective. If you could help add as many tests as possible which can cover all the possibilities of this functionality (with and without priorities), that would be a great help here. I hope that adds clarity to where I am coming from. |
Now this PR also contains changes that is required for #2664, This was needed so I was able to write unit tests in expected order. Lets focus on #2664 first and then we can continue with this discussion after that. |
Hi,
This is a feature that we have been asked by many of our users for years now. And I thought that it might be easier to create a pull request to start the discussion. The code is not ready and lacking allot of test. I just wanted to push a draft that shows that it is possible without to much effort.
Priority on configuration methods is something our users have requested from us for long time, today many of our user control the order by the naming of the methods, but I think it would be much nicer to to reuse the support that already exists in TestNg for Test Methods and I can not see any problem of adding such functionality. Many of our users can have over 50 before/after methods and adding dependency between them make it very complicated to maintain. much easier if the users can group the methods with different priorities.
What is your opinion about adding Priority support for Configuration Methods, if you are ok I will continue with this PR.
I expect it to work in similar way as for Test Annotation.
I tried to bring this up in the forum many years ago, but did not get much response. So I will try here instead.
https://groups.google.com/g/testng-dev/c/qSZFOYRWHkY/m/JzcswgrnAAAJ
The text was updated successfully, but these errors were encountered: