Skip to content

RBAC 테스트 추가 및 정적 엔진 테스트 리팩터링#25

Closed
ikjeong wants to merge 6 commits intomainfrom
test/rbac
Closed

RBAC 테스트 추가 및 정적 엔진 테스트 리팩터링#25
ikjeong wants to merge 6 commits intomainfrom
test/rbac

Conversation

@ikjeong
Copy link
Contributor

@ikjeong ikjeong commented Nov 18, 2025

Summary

  • RBAC 테스트
    go test ./tests/integration -v -run TestValidator_RBAC

  • JavaScript 정적 엔진 테스트
    go test ./tests/integration -v -run TestValidator_JavaScript

  • TypeScript 정적 엔진 테스트
    go test ./tests/integration -v -run TestValidator_TypeScript

  • Java 정적 엔진 테스트
    go test ./tests/integration -v -run TestValidator_Java

- Add createGitChangeFromFile helper using git diff
- Update 18 integration tests to use ValidateChanges instead of Validate
- Remove legacy Validate(path string) method and Result type
- Update test_validator.go to use ValidateChanges
- Unify validation through ValidateChanges for consistency with CLI
- Add environment variable support to git.GetCurrentUser()
  - Checks GIT_AUTHOR_NAME env var first (for testing)
  - Falls back to git config user.name

- Add RBAC-specific test helpers
  - setupRBACEnvironment() - Sets git user via env var
  - createRBACTestValidator() - Creates validator with RBAC config
  - assertRBACViolation() - Verifies RBAC violations occurred
  - assertNoRBACViolation() - Verifies no RBAC violations

- Update createGitChangeFromFile() to use relative paths
  - Converts absolute paths to relative from repo root
  - Matches behavior of real GetGitChanges() function

- Add 10 RBAC integration test cases
  - Admin full access
  - Developer allowed/denied file permissions
  - Viewer denied all access
  - Mixed permissions in single commit
  - Deleted files skipped
  - RBAC disabled mode
  - Unknown user handling
  - Glob pattern matching

- Add RBAC testdata structure
  - Complete test directory with sample files
  - Policy configuration with roles and permissions
  - Test files at various access levels

All tests passing ✓
## Problem
Java integration tests (7/8) were failing with EOF parsing errors:
- Checkstyle/PMD couldn't find test files
- Relative paths were resolved incorrectly
- WorkDir mismatch between test execution and validator

## Root Cause
1. Tests run from `/workspace/tests/integration`
2. Test files located at `/workspace/testdata/java/**`
3. Validator WorkDir set to test directory
4. Adapters tried `/workspace/tests/integration/testdata/java/**` (incorrect)

## Solution

### A. File Path Handling
**Checkstyle Executor** (`internal/adapter/checkstyle/executor.go`)
- Added file existence verification before execution
- Support both absolute and relative paths
- Clear error messages when files not found

**PMD Executor** (`internal/adapter/pmd/executor.go`)
- Same file path improvements as Checkstyle

**Test Helper** (`tests/integration/helper.go`)
- Changed to use absolute file paths in GitChange objects
- Set Validator WorkDir to repo root during creation
- Removed complex relative path conversion logic

### B. Error Handling
**Checkstyle Parser** (`internal/adapter/checkstyle/parser.go`)
- Improved empty output handling
- Better error messages for non-zero exit codes
- Prevents misleading EOF errors

### C. RBAC Compatibility
**Validator** (`internal/validator/validator.go`)
- Convert absolute paths to relative for RBAC checking
- Ensures RBAC patterns still match correctly
- Maintains backward compatibility

### D. Cleanup
**go.mod**
- Fixed import order (go mod tidy)

## Test Results

**Java Tests**: 8/8 PASS ✓
- Pattern violations/valid: 2/2
- Length violations/valid: 2/2
- Style violations/valid: 2/2
- AST violations/valid: 2/2

**RBAC Tests**: 10/10 PASS ✓ (no regression)

**CI Mode**: All tests pass with `-short` flag ✓
All 10 RBAC integration tests now check testing.Short() and skip in CI,
ensuring consistency with other integration tests and preventing them
from running in the CI 'Unit Test' job.
Adds explicit return statements after t.Fatal() calls in nil checks to help
the linter understand that execution stops, preventing SA5011 warnings about
possible nil pointer dereferences.
Completes staticcheck SA5011 fixes by adding return after t.Fatal
in tsc adapter test, ensuring all adapter tests satisfy the linter.
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분 변경이 꼭 필요한 부분이야? 내 PR도 비슷한 수정이 포함되어 잇어

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RBAC 테스트하려다보니 저렇게 넣었는데, 좋은 패턴은 아닌것같아

@ikjeong ikjeong closed this Nov 25, 2025
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.

2 participants