chore(ci): Clean up env variable leak in TestSqlConf#18486
Conversation
geserdugarov
changed the title
TestSqlConf
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
Style & Readability Review — code looks clean overall, but one readability issue: the magic field name "m" in the reflection call needs clarification via a comment.
| } | ||
|
|
||
| def unsetEnv(key: String): String = { | ||
| val field = System.getenv().getClass.getDeclaredField("m") |
There was a problem hiding this comment.
🤖 nit: the field name "m" is a magic constant referring to JDK internals — could you add a comment explaining it's the internal map field of System.getenv()?
- Generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
LGTM — clean, minimal fix that correctly mirrors the existing setEnv reflection pattern to undo the environment variable side-effect, preventing leakage into subsequent test classes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18486 +/- ##
============================================
+ Coverage 68.78% 68.79% +0.01%
- Complexity 28126 28169 +43
============================================
Files 2452 2459 +7
Lines 135059 135114 +55
Branches 16385 16380 -5
============================================
+ Hits 92894 92952 +58
- Misses 34798 34799 +1
+ Partials 7367 7363 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yihua
left a comment
yihua
left a comment
There was a problem hiding this comment.
🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.
LGTM — nice follow-up! The previous review had no open findings, and this update cleanly extracts the magic string "m" into a named constant with a descriptive comment, making the reflection intent much clearer to future readers. No new issues introduced.
* chore(ci): Clean up env variable leak in `TestSqlConf` * clarify magic field name
* chore(ci): Clean up env variable leak in `TestSqlConf` * clarify magic field name
5 participants
Describe the issue this Pull Request addresses
TestSqlConfchanges the JVM's environment variable map, but does not revert those changes in the end.In a result, any test class, which will call
DFSPropertiesConfiguration::refreshGlobalPropsafter, would load from the test'sexternal-config/hudi-defaults.confinstead of the default path.Summary and Changelog
Clean up environment variable map after
TestSqlConfis done.Impact
No
Risk Level
None
Documentation Update
None
Contributor's checklist