Remove outer whitespace from YAML values and test for it in every file by kingthorin · Pull Request #1959 · datafaker-net/datafaker

kingthorin · 2026-11-05T02:24:44Z

Trims leading and trailing whitespace from string values across the YAML data files, and adds YmlOuterWhitespaceTest, which checks every string value in every src/main/resources YAML file so a stray space in any key fails CI, whether or not a provider test references it.

Intentional whitespace is listed in the test's ALLOWED list. Each entry is scoped to one key path and a value pattern, so other values under the same key still fail:

separator: " & ", " і " and similar (joins the last two items of a list)
address.building_number: " s/n.", " #", " ##" (es, ca-CAT)
address.city_suffix: " aan de Rijn" (nl; only a leading space is allowed)
lorem.punctuation.space: the space itself
http.response_body.markdown: a single trailing newline

Other changes:

Removes KpopNamesWhitespaceTest, which the new test covers.
Moves the US postcode format check from net.datafaker.UsPostcodesByStateTest into AddressTest as a nested TestSpec class. The states are read from the postcode_by_state keys, so a newly added state is checked without editing the test.
Adds the same for vehicle models in VehicleTest, with the makes read from the models_by_make keys.
Data fixes found along the way (including "The Cortinas" in en/music.yml and a trailing space in el-GR.yml).

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.49%. Comparing base (8883fa2) to head (68aa771).
⚠️ Report is 5 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1959      +/-   ##
============================================
+ Coverage     92.45%   92.49%   +0.03%     
- Complexity     3581     3585       +4     
============================================
  Files           348      348              
  Lines          7081     7084       +3     
  Branches        694      695       +1     
============================================
+ Hits           6547     6552       +5     
+ Misses          370      368       -2     
  Partials        164      164

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

asolntsev

Thanks for the cleanup! Many padded values are fixed, and the strict-by-default TestSpec check is a nice idea.

I'd ask for the Cortinas fix and a test that proves the new assertion actually fails on a padded value. The other comments are smaller or could be follow-ups.

asolntsev · 2026-11-07T16:02:59Z

@@ -59,7 +59,6 @@ en:
      - "Coldplay"
      - "Cootie Williams"
      - "Cortinas"


The removed " The" was the second half of an inverted "Cortinas, The" that got split into two entries. Deleting it leaves "Cortinas" as a bare name, while the rest of the list uses "The Cramps", "The Clash" and so on. Suggest:

Suggested change

- "Cortinas"

- "The Cortinas"

I think this was a hand made mistake thanks for catching it!

asolntsev · 2026-11-07T16:02:59Z

        - "Πιερικός"
        - "Παναιτωλικός"
-        - "Καστοριά "
+        - "Καστοριά" 


The space was moved outside the closing quote instead of removed, so this line now ends with trailing whitespace. The value is correct, but editorconfig and whitespace linters will flag the line.

Suggested change

- "Καστοριά"

- "Καστοριά"

Hahaha another pesky human error 🤣

asolntsev · 2026-11-07T16:02:59Z

+        if (!testSpec.outerWhitespaceAllowed()) {
+            assertThat(expected)
+                .as(() -> "Values in %s must not have leading or trailing whitespace".formatted(collection))
+                .allMatch(entry -> entry == null || entry.equals(entry.strip()));


entry == null || can never be reached: the noneMatch(String::isBlank) assertion just above already throws on a null entry. Simpler:

Suggested change

.allMatch(entry -> entry == null || entry.equals(entry.strip()));

.allMatch(entry -> entry.equals(entry.strip()));

asolntsev · 2026-11-07T16:02:59Z

        assertThat(expected)
            .as(() -> "Actual should not have empty entries. " + collection)
            .noneMatch(String::isBlank);
+        if (!testSpec.outerWhitespaceAllowed()) {


This check only covers keys that are registered in some TestSpec. Most of the YAML fixes in this PR (oscar_movie, show, babylon5, horse, mass_effect, money_heist, bojack_horseman, planet, hu, ko, uk, pt-BR, el-GR, de, de-CH) were made by hand, and those keys have no spec, so a new "Foo " there would still pass CI. A follow-up idea: one test that walks every string leaf in every src/main/resources YAML, with a small allowlist (nl city_suffix, http markdown bodies), would enforce this everywhere.

Yeah, I debated this a bit while I was doing it. I see different options.

A custom spotless function and exclusion handling

A unit test that iterates all the data files/strings and checks, with exclusion handling

asolntsev · 2026-11-07T16:02:59Z

+
+    /** Verifies that a string without outer whitespace passes the strip check. */
+    @Test
+    void cleanStringPassesStripCheck() {


These tests (and the stripBehavior* ones below) test the JDK's String.strip(), not the new check in AbstractProviderListTest.testProviderList. If that block were deleted or its condition inverted, every test in this class would still pass. Could we add a test that runs testProviderList against a supplier or list containing "foo " and asserts that it fails, plus one with allowOuterWhitespace() that asserts it passes?

asolntsev · 2026-11-07T16:02:59Z

+
+    /** Verifies that the opt-out does not disable other TestSpec validations (e.g., regex). */
+    @Test
+    void otherValidationsStillApply() {


This test never calls allowOuterWhitespace(); it only reads back the regex field it just set, so it doesn't check what its name and javadoc describe.

asolntsev · 2026-11-07T16:02:59Z

+class UsPostcodesByStateTest extends ProviderListTest<BaseFaker> {
+    private static final BaseFaker US_FAKER = new BaseFaker(new Locale("en", "US"));
+
+    private static final List<String> STATES = List.of(


The deleted net.datafaker.UsPostcodesByStateTest iterated over every key of postcode_by_state in _US.yml; this hard-coded list means a state added later (e.g. PR) is silently not checked. Could the specs be generated from the YAML keys instead? VehicleListTest.MAKES has the same issue for models_by_make.

asolntsev · 2026-11-07T16:02:59Z

+    /** Some locales define a key as a scalar placeholder or as templates ("#{...}") instead of plain values, those can't be list-checked. */
+    private static boolean isPlainList(BaseFaker faker, String key) {
+        return faker.fakeValuesService().fetchObject(key, faker.getContext()) instanceof List<?> values
+            && values.stream().noneMatch(value -> String.valueOf(value).contains("#{"));


If any single value in a list contains #{, the whole (locale, key) list is skipped, so its other plain values are never whitespace-checked. Also, a missing or renamed key (fetchObject returns null) is skipped silently instead of failing. A possible fix is to check only the plain values of a mixed list. This line also fetches the list a second time, after testProviderList has already fetched it.

asolntsev · 2026-11-07T16:02:59Z

+    /** Dutch city suffixes such as " aan de Rijn" intentionally start with a space, they are appended to a city name. */
+    private static TestSpec citySuffixSpec(BaseFaker faker) {
+        TestSpec spec = TestSpec.of(faker.address()::citySuffix, "address.city_suffix");
+        return "nl".equals(faker.getContext().getLocale().getLanguage()) ? spec.allowOuterWhitespace() : spec;


allowOuterWhitespace() turns off the check for every nl city_suffix value, so " aan de Rijn " or "Rijn " would pass. A regex such as ^ \S.*\S$ passed to TestSpec.of(..., regex) would allow only the intended leading space.

asolntsev · 2026-11-07T16:02:59Z

@@ -0,0 +1,12 @@
+package net.datafaker.providers.base;


This PR adds 13 *ListTest classes (Azure, Barcode, Code, Credentials, Domain, Educator, File, Internet, Name, Sip, SlackEmoji, Twitter, Vehicle) next to the existing AzureTest, BarcodeTest, ... VehicleTest. The PR already makes MusicTest, VerbTest and FamousLastWordsTest extend BaseFakerTest and override providerListTest(). Doing the same for the others would keep each provider's tests in one file and remove 13 near-identical boilerplate classes.

kingthorin · 2026-11-07T16:11:25Z

Thanks for tye review @asolntsev, I'm teavelling today but I'll tack this in the next few days.

kingthorin · 2026-11-07T16:13:44Z

Converted to draft until I can address the feedback.

Trim leading/trailing whitespace from values across the YAML data files and add YmlOuterWhitespaceTest, which checks every string value in every src/main/resources YAML file. Intentional cases (word separators, building number suffixes, nl city suffixes, the lorem space, markdown trailing newline) are listed in the test with the value pattern they allow. Replace KpopNamesWhitespaceTest with that general check and move the US postcode format check into AddressTest as a TestSpec generated from the postcode_by_state keys. Add the same for vehicle models, generated from the models_by_make keys in VehicleTest. Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>

kingthorin · 2026-11-09T23:08:08Z

@asolntsev I think I got everything done. I undid the TestSpec idea and went ahead with the data file/string iteration.

kingthorin force-pushed the outerwhitespace branch from d6f9491 to 99be711 Compare November 5, 2026 02:40

asolntsev added this to the 3.0.0 milestone Nov 7, 2026

asolntsev reviewed Nov 7, 2026

View reviewed changes

kingthorin marked this pull request as draft November 7, 2026 16:13

kingthorin force-pushed the outerwhitespace branch from 99be711 to 70d3c81 Compare November 9, 2026 22:37

kingthorin changed the title ~~Validate outer whitespace by default in TestSpec and expand provider list coverage~~ Remove outer whitespace from YAML values and test for it in every file Nov 9, 2026

kingthorin force-pushed the outerwhitespace branch from 70d3c81 to 68aa771 Compare November 9, 2026 23:07

kingthorin marked this pull request as ready for review November 9, 2026 23:08

	.allMatch(entry -> entry == null \|\| entry.equals(entry.strip()));
	.allMatch(entry -> entry.equals(entry.strip()));

Conversation

kingthorin commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related

Uh oh!

codecov-commenter commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

asolntsev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingthorin Nov 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingthorin commented Nov 7, 2026

Uh oh!

kingthorin commented Nov 7, 2026

Uh oh!

kingthorin commented Nov 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kingthorin commented Nov 5, 2026 •

edited

Loading

codecov-commenter commented Nov 5, 2026 •

edited

Loading

kingthorin Nov 7, 2026 •

edited

Loading