Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PHOENIX-5230 Fix ChangePermissionsIT and TableDDLPermissionIT on master #477

Closed
wants to merge 1 commit into from
Closed

PHOENIX-5230 Fix ChangePermissionsIT and TableDDLPermissionIT on master #477

wants to merge 1 commit into from

Conversation

twdsilva
Copy link
Contributor

@twdsilva twdsilva commented Apr 5, 2019

@karanmehta93 @swaroopak Please review. I moved all the tests from ChangePermissionsIT, SystemTablePermissionsIT and TableDDLPermissionIT into BasePermissionsIT and created two subclasses one each for namespaces enabled/disabled. I only create a single minicluster for each class.

swaroopak
swaroopak reviewed Apr 5, 2019
View reviewed changes
phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java
import java.util.Set;

import com.google.common.base.Joiner;
import com.google.common.base.Throwables;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the recent jira for logging, let's use org.slf4j.Logger
and org.slf4j.LoggerFactory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed by @yanxinyi in PHOENIX-5228

swaroopak
swaroopak reviewed Apr 5, 2019
View reviewed changes
phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java Outdated

unprivilegedUser = User.createUserForTesting(configuration, "unprivilegedUser", new String[0]);
@Before
public void setUp() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this right after the constructor for better readability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doSetup is called even before the setup happens, is that correct? in that case ignore the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doSetup is called once per class, while setup is called before each test executes. I'll rename these.

swaroopak
swaroopak reviewed Apr 5, 2019
View reviewed changes
phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java Outdated
public class BasePermissionsIT extends BaseTest {

private static final Log LOG = LogFactory.getLog(BasePermissionsIT.class);

static String SUPERUSER;
static String SUPERUSER = System.getProperty("user.name");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: SUPER_USER

@twdsilva
Copy link
Contributor Author

twdsilva commented Apr 8, 2019

@swaroopak I uploaded a new PR based on your feedback, please review.

ChinmaySKulkarni
ChinmaySKulkarni reviewed Apr 8, 2019
View reviewed changes
phoenix-core/src/it/java/org/apache/phoenix/end2end/BasePermissionsIT.java
import static org.junit.Assert.fail;

@Category(NeedsOwnMiniClusterTest.class)
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to fix this order of execution for tests?

@ChinmaySKulkarni
Copy link
Contributor

One question on the FixMethodOrder change. Otherwise, lgtm.

@twdsilva
Copy link
Contributor Author

twdsilva commented Apr 8, 2019

The test aTestRXPermsReqdForPhoenixConn in BasePermissionIT needs to be the very first test that is run

@ChinmaySKulkarni
Copy link
Contributor

@twdsilva is it possible to separate out the part that needs to be run first in aTestRXPermsReqdForPhoenixConn into a private method called from your BeforeClass setup to prevent this ordering dependency?

@twdsilva
Copy link
Contributor Author

twdsilva commented Apr 8, 2019

aTestRXPermsReqdForPhoenixConn tests functionality that assumes it is the very first time a client connects to cluster. I don't think I can move this to BeforeClass.

// NS is disabled, CQSI tries creating SYSCAT, Two cases here
// 1. First client ever --> Gets ADE, runs client server compatibility check again and gets TableNotFoundException since SYSCAT doesn't exist
// 2. Any other client --> Gets ADE, runs client server compatibility check again and gets AccessDeniedException since it doesn't have EXEC perms
            verifyDenied(getConnectionAction(), TableNotFoundException.class, regularUser1);

@ChinmaySKulkarni
Copy link
Contributor

@twdsilva I see. +1

@twdsilva twdsilva closed this Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swaroopak swaroopak swaroopak left review comments

@ChinmaySKulkarni ChinmaySKulkarni ChinmaySKulkarni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@twdsilva @ChinmaySKulkarni @swaroopak