On Thu, Apr 26, 2018 at 3:39 PM Marvin testbot@winehq.org wrote:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Rejected".
This means that the patch has been rejected by a reviewer. You should have received a mail explaining why it was rejected. You need to fix the issue and resend the patch, or if you are convinced that your patch is good as is, you should reply to the rejection message with your counterarguments.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
What is the right way to fix these test failures? Do you think the pre-win10 behavior should be marked as broken?
-Alex
Alex Henrie alexhenrie24@gmail.com writes:
On Thu, Apr 26, 2018 at 3:39 PM Marvin testbot@winehq.org wrote:
Thank you for your contribution to Wine!
This is an automated notification to let you know that your patch has been reviewed and its status set to "Rejected".
This means that the patch has been rejected by a reviewer. You should have received a mail explaining why it was rejected. You need to fix the issue and resend the patch, or if you are convinced that your patch is good as is, you should reply to the rejection message with your counterarguments.
If you do not understand the reason for this status, disagree with our assessment, or are simply not sure how to proceed next, please ask for clarification by replying to this email.
What is the right way to fix these test failures? Do you think the pre-win10 behavior should be marked as broken?
I'm not sure this privilege dropping is meaningful at all. I feel it would be more useful to test more behaviors of that function under normal privileges, with hopefully consistent results across platforms.
On Fri, Apr 27, 2018 at 11:38 AM Alexandre Julliard julliard@winehq.org wrote:
Alex Henrie alexhenrie24@gmail.com writes:
What is the right way to fix these test failures? Do you think the pre-win10 behavior should be marked as broken?
I'm not sure this privilege dropping is meaningful at all. I feel it would be more useful to test more behaviors of that function under normal privileges, with hopefully consistent results across platforms.
You're right. I just tested it and to my surprise, these tests behave the same on all testbot platforms with or without the call to AdjustTokenPrivileges: https://testbot.winehq.org/JobDetails.pl?Key=38082
That makes me think that these are essentially just more invalid parameter tests like the ones my patch already removes. In that light, would it be acceptable to simply delete the test_noprivileges function and encourage anyone working on message broadcasting to write more meaningful tests in the future?
-Alex
Alex Henrie alexhenrie24@gmail.com writes:
On Fri, Apr 27, 2018 at 11:38 AM Alexandre Julliard julliard@winehq.org wrote:
Alex Henrie alexhenrie24@gmail.com writes:
What is the right way to fix these test failures? Do you think the pre-win10 behavior should be marked as broken?
I'm not sure this privilege dropping is meaningful at all. I feel it would be more useful to test more behaviors of that function under normal privileges, with hopefully consistent results across platforms.
You're right. I just tested it and to my surprise, these tests behave the same on all testbot platforms with or without the call to AdjustTokenPrivileges: https://testbot.winehq.org/JobDetails.pl?Key=38082
That makes me think that these are essentially just more invalid parameter tests like the ones my patch already removes. In that light, would it be acceptable to simply delete the test_noprivileges function and encourage anyone working on message broadcasting to write more meaningful tests in the future?
Yes, I think that's acceptable. Also we'll probably want to ask that hypothetical future person to merge the remaining tests into message.c ;-)
-
Alex Henrie -
Alexandre Julliard