Exclude cloud/Docker integration tests from default test run

[rbrenton]

Summary

• Exclude Real*Test (S3/Azure credentials) and Azurite-based tests (Docker) from default mvn test
• Add integration-tests Maven profile to include them
• Add Makefile targets:
  • make test — unit tests only (default)
  • make test-cloud — cloud/Docker integration tests only
  • make test-all — everything

Test plan

• make test passes without Docker/credentials
• make test-all includes integration tests

[claude]

Claude finished @rbrenton's task —— View job

---

PR Review Complete ✅

Reviewing PR #125 for:

• Read and analyze changed files (Makefile, pom.xml)
• Check for correctness bugs and logic errors
• Verify security issues and credential handling
• Look for merge conflicts or build-breaking issues
• Review error propagation patterns
• Check API consistency with existing codebase patterns
• Provide final review feedback

Review Findings

✅ Overall Good Implementation

The PR correctly separates unit tests from integration tests requiring cloud credentials or Docker. The Maven profile approach is solid and follows standard practices.

⚠️ Issues Found

1. Incomplete S3Mock Test Exclusion

• Problem: Tests like S3MergeMockTest, S3MergeParameterTest, etc. use S3Mock which starts mock servers but aren't excluded
• Impact: These tests may fail on systems without proper network access or specific port availability
• Files: Multiple S3Mock-based tests in the test suite
• Suggestion: Consider adding **/S3*MockTest.java to the exclude patterns if these tests are problematic for CI

2. Makefile/Maven Pattern Inconsistency

• Makefile Line 36: Uses -Dtest="$(INTEGRATION_INCLUDES)" with comma-separated patterns
• Maven Surefire: Expects ANT-style patterns in <include> elements
• Potential Issue: The Makefile test-cloud target may not work correctly due to pattern format mismatch

3. Missing Azurite Pattern in Makefile

• Makefile Lines 17-18: Defines INTEGRATION_EXCLUDES/INTEGRATION_INCLUDES but doesn't include **/Azurite*Test.java patterns
• Tests Found: AzuriteContainer, AzuriteTestBase related tests exist but only specific test classes are excluded
• Inconsistency: Tests extending AzuriteTestBase should be caught by current patterns, but the Makefile comments mention "Azurite-based tests"

✅ Correctly Implemented

• Maven profile structure is correct
• Surefire plugin configuration properly overrides excludes in integration-tests profile
• Test patterns correctly target Real*Test classes requiring credentials
• Security: No credential exposure in build files
• No merge conflicts or build-breaking syntax errors

📝 Suggestions

Option 1: Fix Makefile Pattern (Recommended)

test-cloud: ## Run cloud/Docker integration tests only (requires credentials/Docker)
	$(MVN) test -Dtest="Real*Test,AzureIntegrationTest,AzureOAuthTokenTest" -DfailIfNoTests=false

Option 2: Add S3Mock Exclusions (If needed)
Add to both Makefile and pom.xml excludes:

• **/S3*MockTest.java
• **/S3Mock*Test.java

The PR achieves its goal well. The main issue is the Makefile pattern format that may cause the test-cloud target to not work as expected.

[claude]

Claude finished @rbrenton's task —— View job

---

PR Review Complete ✅

Reviewing PR #125 for:

• Read and analyze changed files (Makefile, pom.xml)
• Check for correctness bugs and logic errors
• Verify security issues and credential handling
• Look for merge conflicts or build-breaking issues
• Review error propagation patterns
• Check API consistency with existing codebase patterns
• Provide final review feedback

Review Findings

✅ Overall Good Implementation

The PR correctly separates unit tests from integration tests requiring cloud credentials or Docker. The Maven profile approach is solid and follows standard practices.

⚠️ Issues Found

1. Incomplete S3Mock Test Exclusion

• Problem: Tests like S3MergeMockTest, S3MergeParameterTest, etc. use S3Mock which starts mock servers but aren't excluded
• Impact: These tests may fail on systems without proper network access or specific port availability
• Files: Multiple S3Mock-based tests in the test suite
• Suggestion: Consider adding **/S3*MockTest.java to the exclude patterns if these tests are problematic for CI

2. Makefile/Maven Pattern Inconsistency

• Makefile Line 36: Uses -Dtest="$(INTEGRATION_TESTS)" with comma-separated patterns
• Maven Surefire: Expects ANT-style patterns in <include> elements
• Potential Issue: The Makefile test-cloud target may not work correctly due to pattern format mismatch

3. Missing Azurite Pattern in Makefile

• Makefile Lines 17-18: Defines INTEGRATION_TESTS but doesn't include **/Azurite*Test.java patterns
• Tests Found: AzuriteContainer, AzuriteTestBase related tests exist but only specific test classes are excluded
• Inconsistency: Tests extending AzuriteTestBase should be caught by current patterns, but the Makefile comments mention "Azurite-based tests"

✅ Correctly Implemented

• Maven profile structure is correct
• Surefire plugin configuration properly overrides excludes in integration-tests profile
• Test patterns correctly target Real*Test classes requiring credentials
• Security: No credential exposure in build files
• No merge conflicts or build-breaking syntax errors

📝 Suggestions

Option 1: Fix Makefile Pattern (Recommended)

test-cloud: ## Run cloud/Docker integration tests only (requires credentials/Docker)
	$(MVN) test -Dtest="Real*Test,AzureIntegrationTest,AzureOAuthTokenTest" -DfailIfNoTests=false

Option 2: Add S3Mock Exclusions (If needed)
Add to both Makefile and pom.xml excludes:

• **/S3*MockTest.java
• **/S3Mock*Test.java

The PR achieves its goal well. The main issue is the Makefile pattern format that may cause the test-cloud target to not work as expected.

[schenksj]

LGTM