@bhroam: If user is setting a boolean to 1 and in manager we should also ask user to enter expected value after setting.
For e.g,
manager(SET, {‘scheduling’:1}, expect={‘scheduling’:True}, op=EQ)
with the above example, user is telling to set scheduling to 1 and expecting value to be True both validation is done by manager itself unless expect is set. if user doesn’t provide any value to expect (as default it will be True) PTL will expect for value to be 1 and test fails.
@vishwaks how would you handle INCR and DECR operator where you are adding or removing a value from the string?
What you are proposing looks simple. The only issue would be if tomorrow we change any of the value (which is highly unlikely) then we have to make a change to the test.
Those can be handled using op=IN and NOTIN.
And on updating test - Yeah we have to. right ? Even the values of setting attributes can change.
IMHO, this is redundant. We’re having the test writer set it, and then say what they expect. If that was the case, test writers would just do it once and set it to scheduling=True.
Right now we do one expect() passing all of the attributes in one go. We won’t be able to do that if we need to use different operators.
I thought of a new option that I’m not sure I like or not. I’ll throw it out to see what people think. Right now manager() will throw an exception if it fails. In the case it succeeded, do we really need to be pedantic and check if it was set properly? Can we trust PBS to have set it properly? In that case we could then remove the expect argument from manager(). The only case I can see where you want to see if something is set properly is if you are doing something strange in an action function (like queue_type=e turning into Execution). In that case, you’ll want to check for something special anyway. I guess one other point to make is that it took us years to notice that expect=True always passed. If that was the case, do we really need it?
Bhroam
Yeah, it will be redundant for special cases like scheduling(set 1) and queue_type(set e).
And in my opinion YES for removing expect=True from manager().
Ok, but we’ll then need to find some other way to ensure that when somebody sets scheduling to False, PTL will ensure that server is done with the current scheduling cycle before returning control back to the user. That’s handled by expect right now.
Also, you started this discussion by mentioning that you wanted expect to check that the attributes were not only set, but that they were set to the correct value. Have you since changed your mind about wanting that? Or was expect’s existing behavior confusing and that’s what motivated you to start this discussion (as opposed to wanting expect to do more)?
@agrawalravi90 my initial intent was to fix the bug that expect=True always passed. The more and more I think about it, I’m not sure expect is even needed. If manager() is going to raise an exception when it fails, do we really need to check if things are set properly? I think we can trust that PBS did the right thing, or reported an error if not. You are right that we’d need to still fix the issue with scheduling=False waiting until the cycle is over, but my thought was to have that always happen. Is there a reason to ever not wait until a cycle is over? We’ll need to add an obscene max_attempts though. If using this in a performance test, you’ll need to wait a long time for a cycle to finish.
Bhroam
hmm … Ok, I see your point, and I honestly I can’t think of a reason to have expect there besides checking for the scheduler to be idle, so I think I like your approach.
We could provide an option to make manager() non-blocking in case the user just wanted PTL to fire off the PBS command and then return without doing any checks.