test(config): add unit test coverage for ConfigParserUtil#5659
Merged
Conversation
Pins behavior of the size-string parser used by StorageConfig for
S3 multipart sizing:
- unit multipliers (KB / MB / GB), multi-digit values, Long-precision
GB results that overflow Int
- decimal interpretation of leading zeros (no octal)
- IllegalArgumentException for missing unit, unsupported unit (TB),
empty string, lowercase unit, embedded whitespace, non-numeric
value, unit-only input
Adds scalatest as a Test dependency in common/config/build.sbt — the
module had no test infrastructure before this PR (no src/test
directory existed); scalatest is configured with `% Test` so it
doesn't leak into the production classpath. Version 3.2.15 matches
the other backend modules (common/workflow-operator, common/dao,
amber/build.sbt).
Closes apache#5655
github-actions
Bot
added
dependencies
Contributor
There was a problem hiding this comment.
Pull request overview
Adds ScalaTest-based unit coverage to the common/config module to pin and protect the parsing contract of ConfigParserUtil.parseSizeStringToBytes, which is used by StorageConfig for S3 multipart part sizing.
Changes:
- Introduces
ConfigParserUtilSpecwith targeted cases for valid units, multi-digit inputs, leading zeros, and malformed inputs. - Adds ScalaTest (
org.scalatest%Test) tocommon/config’sbuild.sbtand creates the module’s initialsrc/testlayout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common/config/src/test/scala/org/apache/texera/amber/util/ConfigParserUtilSpec.scala | New ScalaTest spec covering parseSizeStringToBytes success and failure behaviors. |
| common/config/build.sbt | Adds ScalaTest test-scope dependency to enable unit testing in common/config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5659 +/- ##
============================================
- Coverage 52.96% 52.55% -0.42%
- Complexity 2522 2543 +21
============================================
Files 1075 1089 +14
Lines 41743 42131 +388
Branches 4513 4529 +16
============================================
+ Hits 22109 22140 +31
- Misses 18329 18678 +349
- Partials 1305 1313 +8
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
✅ No material benchmark regressions detected🟢 15 better · 🔴 0 worse · ⚪ 0 noise (<±5%) · 0 without baseline
Baseline detailsLatest main
Raw CSVconfig_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,457.92,200,128000,437,0.267,22511.76,32866.85,32866.85
1,100,10,64,20,2138.07,2000,1280000,935,0.571,104939.84,133210.53,133210.53
2,1000,10,64,20,17824.58,20000,12800000,1122,0.685,904232.84,944905.53,944905.53 |
common/config had no tests until this PR, so the amber job's jacoco invocation explicitly skipped it. Add Config/jacoco so ConfigParserUtilSpec actually runs in CI and the module's jacoco.xml / test-reports are picked up by the existing Codecov upload globs.
Yicong-Huang
approved these changes
Jun 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
Pin behavior of
ConfigParserUtil.parseSizeStringToBytes— the size-string parser used byStorageConfigfor S3 multipart sizing. Thecommon/configmodule had no test infrastructure before this PR (nosrc/testdirectory existed); this PR adds the directory and configures the standard ScalaTest dependency the way the other backend modules do.ConfigParserUtilSpecConfigParserUtilSpec file name follows the
<srcClassName>Spec.scalaone-to-one convention.Behavior pinned
1KB/1MB/1GB1024L/1024 * 1024L/1024 * 1024 * 1024L100MB,1024KB,128GB)5GBLongprecision (result exceedsInt.MaxValue)0010KB10 * 1024L(decimal-only, no octal interpretation)100)IllegalArgumentExceptionwith diagnostic5TB)IllegalArgumentExceptionIllegalArgumentException5mb)IllegalArgumentException(regex is anchored to[KMG]B)5 MB)IllegalArgumentExceptionabcMB)IllegalArgumentExceptionMB)IllegalArgumentExceptionLong(compile-time enforced)Build-config change
Adds
org.scalatest %% scalatest % 3.2.15 % Testtocommon/config/build.sbt. The version matches the other backend modules (common/workflow-operator,common/dao,amber). Scope isTestso the dependency does not leak into the production classpath.Any related issues, documentation, discussions?
Closes #5655.
How was this PR tested?
Pure unit-test addition (plus the build-config tweak above); verified locally with:
sbt "Config/testOnly org.apache.texera.amber.util.ConfigParserUtilSpec"— 16 tests, all greensbt scalafmtCheckAll— cleanWas this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7 [1M context])