Skip to content

add auth test UT#2271

Merged
qiaojialin merged 11 commits intomasterfrom
refactor_auth_test
Dec 18, 2020
Merged

add auth test UT#2271
qiaojialin merged 11 commits intomasterfrom
refactor_auth_test

Conversation

@Genius-pig
Copy link
Copy Markdown
Contributor

add auth test UT

Copy link
Copy Markdown
Contributor

@Alima777 Alima777 left a comment

Choose a reason for hiding this comment

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

Try to modify all test named test() or add javadoc for it. The naming method is sooo ambiguous and not intuitive to maintain.

Comment on lines +57 to +59

@Test
public void test() throws AuthException, IllegalPathException {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add javadoc to explain what the test is about, and try not to name a test as just test()

Comment on lines +62 to +65
PathPrivilege pathPrivilege = new PathPrivilege();
Set<Integer> set = new HashSet<>();
set.add(1);
pathPrivilege.setPrivileges(set);

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

update

Comment on lines +127 to +133
Assert.assertFalse(AuthorityChecker.check(user.getName(), Collections.singletonList(new PartialPath(nodeName)),
OperatorType.CREATE_TIMESERIES, user.getName()));

Assert.assertFalse(AuthorityChecker.check(user.getName(), Collections.singletonList(new PartialPath(nodeName)),
OperatorType.DELETE_TIMESERIES, user.getName()));
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not a good test. The user created has no privilege at all, so all assertions return false. The correct test method may be: 1. test the authority before granting, return false. 2. grant the privilege 3. test the authority again, return true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

update

import org.junit.Before;
import org.junit.Test;

public class LocalFileAuthorizerTest {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class LocalFileAuthorizerTest {
public class BasicAuthorizerTest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think BasicAuthorizerTest is better..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In fact, BasicAuthorizer getInstance return a LocalFileAuthorizer instance

Genius-pig and others added 6 commits December 17, 2020 13:51
…dAuthorizerTest.java

Co-authored-by: Xiangwei Wei <34242296+Alima777@users.noreply.github.com>
…java

Co-authored-by: Xiangwei Wei <34242296+Alima777@users.noreply.github.com>
…dAuthorizerTest.java

Co-authored-by: Xiangwei Wei <34242296+Alima777@users.noreply.github.com>
…java

Co-authored-by: Xiangwei Wei <34242296+Alima777@users.noreply.github.com>
…legeTest.java

Co-authored-by: Xiangwei Wei <34242296+Alima777@users.noreply.github.com>
@Alima777
Copy link
Copy Markdown
Contributor

Hi, check the CI please.

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 11.
Read more here

@qiaojialin qiaojialin merged commit e97d532 into master Dec 18, 2020
@qiaojialin qiaojialin deleted the refactor_auth_test branch December 18, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants