PP-1281: New decorator in PTL using which user can provide cluster information required for a test

Developers
1

Hi All,

Please review the design document created at:
https://pbspro.atlassian.net/wiki/spaces/PD/pages/458883073/PP-1281+New+decorator+in+PTL+using+which+user+can+provide+cluster+information+required+for+a+test.

Please do let me know of any comments or suggestions.

Thanks and Regards,
Sanket

2

Hey @borlesanket,
I’m curious on what the future plans for this decorator will be. From what I can see now, it’s meant to set the minimum requirements for the test. If this is the whole purpose of this decorator, I think @clusterinfo might be a confusing name.

Maybe @requirements()?

3

@bhroam, In future we may specify platforms(centos, rhel, sles etc.), build etc. along with number of daemons, that’s why I kept it as clusterinfo. But I see that would also be the requirement for the test, so @requirements seems to be better name . What do you think?

4

I personally like @requirements() better. Maybe @test_requirements()? It makes more sense to me than @clusterinfo().

Thanks for posting the design document. I think this will be a nice extension to PTL.

5

Hi Sanket,

In the interface details section you are talking about internal design. Do we need that here?
Also I am not clear if @clusterinfo (or @test_requirements) decorator is also going to skip the tests if insufficient values provided for mom, comm, etc …
Another thing that is not clear is how do I specify which mom has comms. For example, I have 4 moms but only 2 has comms.

could you add the example of the format for a test class and test case for PTLTestInfo._gen_ts_tree()?

6

Thanks @bhroam, I have changed it to “@TestRequirements”.

@anamika,
1.In couple of last EDD, I have been asked to put the internal design too. That’s why I have added it.
2.This decorator is just a metadata, it will not do any validations and will not skip the tests. This is just a information which other tools will use to form a cluster while running the testcase. For the requirement of skipping the test, we may extend the skip decorator in future which we just added in PTL.
3.You just specify the number of comm’s as 2 and moms as 4. I have added couple of examples in document.
4.I have added a format for PTLTestInfo._gen_ts_tree() as a example in document.
Please have a look at the document and let me know if this clarifies your doubts.

7

Hi Sanket,

2.This decorator is just a metadata, it will not do any validations and will not skip the tests. This is just a information which other tools will use to form a cluster while running the testcase. For the requirement of skipping the test, we may extend the skip decorator in future which we just added in PTL.

Anamika -> Please make this clear in the details/synopsis.

Rest looks good.

8

I don’t think this is right.
Two reason:

  1. If user has installed pbspro-execution RPM then there will be no pbs_comm binary in PBS_EXEC/sbin. Also there will be no PBS_HOME/server_priv and PBS_HOME/comm_logs dir which is required by pbs_comm. Only pbspro-server RPM has pbs_comm binary (see here and here)
  2. What if user want 4 moms and 2 comms on separate machine? Means total 7 machine 1 for Server/Sched, 4 for moms and 2 for comms?
9

@hirenvadalia,

  1. If that is the case then we should install server rpm for the comm which is on different node than server and make only mom and comm as 1 in pbs.conf on that node.
  2. In this case then, we need one more boolean as ‘nocommonmom’, what do you say?

@anamika, Thanks. I have updated the document accordingly.

10

I understand the skip decorator has already been merged, but I wonder if we need two decorators. Right now the skip decorator doesn’t have any requirements to skip, but it is the intension of adding them later. Once that happens, you’ll need this decorator saying I need 2 moms and the skip decorator saying to skip if I don’t have 2 moms.

Should this decorator be merged into the skip decorator (or vice versa)?

Bhroam

11

@bhroam, we can merge this into skip decorator. But what I feel is, these two are logically different, in skip we are telling to skip the testcase which will happen at run time, where as requirements is just a metadata which other tools will use to get the cluster information. Now if we want to validate/skip according to cluster information then we can add validation inside requirements decorator and can call skip inside it instead of merging it with skip decorator. Let me know your opinion about this. @anamika, you can also provide your opinion about this.

12

Hi @borlesanket, I like the idea of adding validation inside requirements decorator. This way user won’t have to worry about validating separately.

13

I see the logical difference between the two decorators, but the reality of it all is that the decorators will contain the same information. If we separate them into two different decorators, then people will always have to use both decorators instead of just one. If the skip decorator is the wrong decorator to keep, we could merge it into the requirements decorator and if someone wants a skip decorator, they can use the decorator that is provided by unittest itself.

14

Hey @bhroam, we still have use cases to skip the tests for other reasons besides configuration/requirements like test broken for some releases, test not supported on certain platforms.
The new requirements decorator do not cover platforms. So keeping both the decorators would still be a good idea.

15

@anamika, I can see the need of a skip for a generic reason. For this we can use the decorator provided to us from unittest(https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures). I’d say everything else really is a requirement of the test. Your example of a particular platform sounds like a requirement to me. If a test can only be run on a cray, it requires cray.

1 Like
16

Hi @bhroam, agreed that unitest skip decorator can be used for other skip requirements.
Also for platforms, I see 2 use cases: skip on platformX and run only on platformX. I guess it is a question of how to implement it but it can be satisfied by requirements decorator.

17

@bhroam, @anamika, So from above discussion what I understand is we should remove skip and have @requirements as a single decorator which can be used to get cluster information for now and in future we will add platform/build etc. conditional support into it. And apart from this if user want he can make use of unittest’s skip decorators.

1 Like
18

@borlesanket that sounds good to me.

19

@bhroam, I have updated the document accordingly.
@hirenvadalia, I have added one more boolean as “nocommonmom” to satisfy the condition you have mentioned in your comment (point 2). Please have a look.

20

@borlesanket I am fine with nocommonmom options, But I partially agree on reply to my first point.

I don’t like this as default, mean by default I would like go for default PBS behavior (comm and mom on separate machine) which means new option nocommonmom to True as default instead False.

Also if everyone is ok then I would prefer to see new decorator name as “requirements” instead “TestRequirements”

Few cosmetic comments:
Replase FALSE/TRUE with False/True
Replace @TestRequirements with @testrequirements or @test_requirements (if we don’t go for @requirements)
For better readability rename below options of new decorator:

  • nomomonserver to no_mom_on_server
  • nocommonserver to no_comm_on_server
  • nocommonmom to no_comm_on_mom
1 Like