[grpc logger] reduce GRPC log level by default for tests

[liran-funaro]

Type of change

• Bug fix
• Test update

Description

• This PR adds -race flag for the tests and fix the race conditions
• Fix minor race condition in test
• Use gotestsum to shorten the logs
• Reduce GRPC log level by default using environment variable instead of manually

Root Cause Analysis: Two Data Races Fixed by This PR

1. Test Data Race in TestLoadPrivateKey_BadPEM

• Cause: Multiple parallel subtests shared a parent-scope err variable
• Manifestation: Write-write and read-write conflicts at lines 86-87
• Fix: Changed _, err = to _, loadErr := to create local variable per subtest

2. gRPC Logger Initialization Race

• Cause: grpclog.SetLoggerV2() is not thread-safe; multiple goroutines could call it concurrently during package initialization
• Manifestation: Concurrent writes to gRPC's global logger state when multiple packages import flogging simultaneously

Related issues

• [data-race] Setting the global GRPC logger while starting the server in another test fabric-x-committer#448

[coveralls]

Changes unknown
when pulling bc67788 on liran-funaro:grpc-logger
into ** on hyperledger:main**.

[cendhu]

Bob's Review Summary

This PR correctly identifies and fixes two race conditions while improving test infrastructure. The changes are well-executed and safe to merge.

✅ Key Strengths

1. Accurate Root Cause Analysis: Both race conditions (test variable shadowing + gRPC logger initialization) are correctly identified and fixed at the source
2. Comprehensive Fix: Removes the problematic util.MustGetLogger wrapper that caused concurrent grpclog.SetLoggerV2() calls
3. Mechanical Refactoring: 50+ files updated consistently to use flogging.MustGetLogger() directly
4. Improved Test Infrastructure:
   • Adds -race flag for race detection
   • Splits CI into test-race and test-cover jobs for better performance
   • Replaces gotestfmt with more modern gotestsum
5. No Breaking Changes: All changes are internal (test infrastructure + logging initialization)

🔍 Technical Analysis

Race #1 - Test Variable Shadowing (tools/cryptogen/csp_test.go):

• Problem: Parallel subtests shared parent-scope err variable
• Fix: Changed to loadErr := for local scope per subtest
• Why it works: Each parallel subtest now operates on independent memory

Race #2 - gRPC Logger Initialization:

• Problem: grpclog.SetLoggerV2() is not thread-safe; called on every logger creation
• Fix: Deleted common/util/logging.go wrapper; use flogging.MustGetLogger() directly
• Why it works: flogging.MustGetLogger() is thread-safe; GRPC log level now set via env var at startup

📊 Impact Assessment

• Security: No security impact; fixes improve system reliability
• Performance: No runtime impact; minor test overhead from -race flag (mitigated by separate CI job)
• Architecture: Simplifies dependency graph by removing unnecessary wrapper
• Documentation: No updates needed (internal changes only)

✅ Recommendation: APPROVE

The PR is ready to merge. All changes are correct, well-tested, and improve code quality.

Post-merge monitoring: Watch for any new race conditions in subsequent PRs; verify test execution time remains acceptable.

---

Full detailed review available in: PR_95_REVIEW.md

[cendhu]

Detailed inline review comments on key changes

[cendhu]

LGTM.

[cendhu]

While running test locally, should we set this env explicitly or set by make target?

[liran-funaro]

I set it manually. The same way I set export DB_DEPLOYMENT=local for my test env.