mesos-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Greg Mann (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (MESOS-6181) The logic for BadACLNoPrincipal and BadACLDropCreateAndDestroy is not correct
Date Mon, 26 Sep 2016 20:58:20 GMT

    [ https://issues.apache.org/jira/browse/MESOS-6181?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15524145#comment-15524145
] 

Greg Mann edited comment on MESOS-6181 at 9/26/16 8:57 PM:
-----------------------------------------------------------

[~gyliu], sorry for my delayed reply! Regarding the use of '{}' to enclose sections of the
test code, I like to do this purely for readability. i.e., instead of re-assigning the value
of {{volume}} several times in a test, it can be nice to declare the variable each time, enclosing
it in a scope to make it obvious where that volume is used, and where it goes out of scope.
If we use a single variable and re-assign its value multiple times in the test without using
scopes, I think it's more difficult to tell what the variable contains at a given point in
time.

However, this particular case isn't a very compelling use of this pattern, I'd say. Unfortunately,
we can't enclose the second volume-creation in a scope, since we reference that volume at
the end of the test when we call {{EXPECT_TRUE(Resources(offer.resources()).contains(volume));}}.

Nonetheless, I believe the logic in the test is correct. Regarding your point #1, the driver
doesn't decline a non-existent offer, since the {{offer}} variable is declared outside of
the '{}' scope, it can be assigned a new value within the scope and that value will still
be accessible after the scope is exited. Regarding point #2, the tests do actually test to
make sure the final offer contains the volume, using {{EXPECT_TRUE(Resources(offer.resources()).contains(volume));}}.

What do you think?


was (Author: greggomann):
[~gyliu], sorry for my delayed reply! Regarding the use of '{' and '}' to enclose sections
of the test code, I like to do this purely for readability. i.e., instead of re-assigning
the value of {{volume}} several times in a test, it can be nice to declare the variable each
time, enclosing it in a scope to make it obvious where that volume is used, and where it goes
out of scope. If we use a single variable and re-assign its value multiple times in the test
without using scopes, I think it's more difficult to tell what the variable contains at a
given point in time.

However, this particular case isn't a very compelling use of this pattern, I'd say. Unfortunately,
we can't enclose the second volume-creation in a scope, since we reference that volume at
the end of the test when we call {{EXPECT_TRUE(Resources(offer.resources()).contains(volume));}}.

Nonetheless, I believe the logic in the test is correct. Regarding your point #1, the driver
doesn't decline a non-existent offer, since the {{offer}} variable is declared outside of
the '{}' scope, it can be assigned a new value within the scope and that value will still
be accessible after the scope is exited. Regarding point #2, the tests do actually test to
make sure the final offer contains the volume, using {{EXPECT_TRUE(Resources(offer.resources()).contains(volume));}}.

What do you think?

> The logic for BadACLNoPrincipal and BadACLDropCreateAndDestroy is not correct
> -----------------------------------------------------------------------------
>
>                 Key: MESOS-6181
>                 URL: https://issues.apache.org/jira/browse/MESOS-6181
>             Project: Mesos
>          Issue Type: Bug
>            Reporter: Guangya Liu
>            Assignee: Guangya Liu
>
> Two issues for those two test cases:
> 1) No need to add `{}` in the test case as there is no need to add `{}`, adding the `{}`
will cause the driver decline a non exist offer.
> 2) If destroy volume failed, we should get the last offer to make sure that the last
offer also contain the volume resource.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message