Refactor tests extending `BaseFakerTest` by asolntsev · Pull Request #1606 · datafaker-net/datafaker

asolntsev · 2025-08-12T19:21:09Z

Remove extends BaseFakerTest from all test that don't need to run testProviderList and testNoDuplications
Simplify few tests
Remove setFaker()

codecov-commenter · 2025-08-12T19:25:58Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.41%. Comparing base (91d3e16) to head (e42bc15).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1606      +/-   ##
============================================
+ Coverage     92.40%   92.41%   +0.01%     
- Complexity     3356     3357       +1     
============================================
  Files           331      331              
  Lines          6619     6618       -1     
  Branches        655      655              
============================================
  Hits           6116     6116              
- Misses          345      346       +1     
+ Partials        158      156       -2

☔ View full report in Codecov by Sentry.
📢 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.

…testProviderList` and `testNoDuplications`. Also, force subclasses to implement `providerListTest()` by marking it as `abstract`. This simplifies understanding of what these tests verify.

instead, just implement method `getFaker()`.

* don't use annotations `@Spy` and `@Mock` * don't need to reset mocks between tests Especially `FakeValuesServiceTest` got much simpler. It's not needed to mock so much. It's always better to use real objects instead of mocks.

Copilot

Pull Request Overview

This PR refactors test classes by removing unnecessary inheritance from BaseFakerTest and AbstractFakerTest for tests that don't need the common test methods (testProviderList and testNoDuplications). The refactoring simplifies test classes by having them directly instantiate Faker instances instead of extending base test classes.

Removes inheritance from BaseFakerTest/AbstractFakerTest for tests that don't use provider list testing
Replaces setFaker() calls with direct getFaker() overrides in nested test classes
Adds direct Faker instance creation in simplified test classes

Reviewed Changes

Copilot reviewed 73 out of 73 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
AbstractFakerTest.java	Completely removed as it's no longer needed
BaseFakerTest.java	Made abstract and removed `setFaker()` method
Various test classes	Removed inheritance and added direct Faker instantiation
Provider test classes	Made abstract and added final modifiers to getFaker()
Nested test classes	Replaced setup/teardown with getFaker() overrides

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-12T20:27:21Z

src/test/java/net/datafaker/providers/foods/HebrewFoodTest.java

 import static org.assertj.core.api.Assertions.assertThat;

 final class HebrewFoodTest extends FoodFakerTest {
    private final Food food = getFaker().food();


The field food calls getFaker().food() but this class no longer extends a base class with getFaker() method. This will cause a compilation error.

Suggested change

private final Food food = getFaker().food();

private final Food food = new Faker().food();

Copilot · 2025-08-12T20:27:22Z

src/test/java/net/datafaker/providers/foods/HebrewFoodTest.java


 import static org.assertj.core.api.Assertions.assertThat;

 final class HebrewFoodTest extends FoodFakerTest {


This class extends FoodFakerTest but the field initialization on line 12 suggests it was meant to be refactored to not extend the base class. Either remove the inheritance or add a getFaker() override for Hebrew locale.

Copilot · 2025-08-12T20:27:22Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java

-    private FakerContext context;
+    private final Faker faker = spy(new Faker(new Locale("test")));
+    private final FakerContext context = faker.getContext();
+    private final FakeValuesService fakeValuesService = new FakeValuesService();


The FakeValuesService constructor requires a FakerContext parameter but none is provided here. This will cause a compilation error.

Suggested change

private final FakeValuesService fakeValuesService = new FakeValuesService();

private final FakeValuesService fakeValuesService = new FakeValuesService(context);

Copilot · 2025-08-12T20:27:22Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java

    @Test
    void bothify2Args() {
-        final DummyService dummy = mock(DummyService.class);
+        DummyService dummy = new DummyService();


The DummyService constructor now requires parameters (based on line 359) but this instantiation provides none. This will cause a compilation error.

Suggested change

DummyService dummy = new DummyService();

DummyService dummy = new DummyService(null); // TODO: replace null with appropriate constructor arguments

Copilot · 2025-08-12T20:27:22Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java

    @Test
    void regexifyDirective() {
-        final DummyService dummy = mock(DummyService.class);
+        DummyService dummy = new DummyService();


The DummyService constructor now requires parameters but this instantiation provides none. This will cause a compilation error.

Suggested change

DummyService dummy = new DummyService();

DummyService dummy = new DummyService(context);

Copilot · 2025-08-12T20:27:22Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java

    @Test
    void regexifySlashFormatDirective() {
-        final DummyService dummy = mock(DummyService.class);
+        DummyService dummy = new DummyService();


The DummyService constructor now requires parameters but this instantiation provides none. This will cause a compilation error.

Suggested change

DummyService dummy = new DummyService();

DummyService dummy = new DummyService(context);

Copilot · 2025-08-12T20:27:23Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java

    @Test
    void regexifyDirective2() {
-        final DummyService dummy = mock(DummyService.class);
+        DummyService dummy = new DummyService();


The DummyService constructor now requires parameters but this instantiation provides none. This will cause a compilation error.

Suggested change

DummyService dummy = new DummyService();

DummyService dummy = new DummyService(context);

kingthorin · 2025-08-12T20:51:47Z

Thanks!!

snuyanzin · 2025-08-12T22:36:34Z

src/test/java/net/datafaker/providers/base/InternetTest.java

+        assertThat(mockedFaker.internet().username())
+            .doesNotContain(" ", "'")
+            .matches("^(\\w+)\\.(\\w+)$")
+            .matches("^\\p{javaLowerCase}+\\.\\p{javaLowerCase}+$");


ideally this test should be rewritten

currently it doesn't check much
If I replace implementation of username with return a.a it will pass

even more: if you compare output of
firstName() + "." + lastName() vs javaLowerCase for internet().username() it will not match because tests are running in TR locale
first will give
jin.quan
second
jın.quan (with turkish dotless i)

Emmmm...

currently it doesn't check much
Yes, this is true for most DF tests. It's because they call random generator and cannot fully verify the random output.

Anyway, now this test checks more than before.

it will not match because tests are running in TR locale

Sorry, I don't understand how to make this test fail. For me, it works with both English and Turkish locale.

It isn't lower casing the value(s), it's comparing against a character set of all Java lower case characters. (From my understanding)

@kingthorin Yes. username returns lower case, and .matches("...\p{javaLowerCase}...") verifies that this is lower case.

@asolntsev

Sorry, I don't understand how to make this test fail.

more details are at

#1669

snuyanzin · 2025-08-12T22:52:17Z

src/test/java/net/datafaker/service/FakeValuesServiceTest.java


        public String hello() {
-            return "Hello";
+            return greetings.get(counter.getAndIncrement());


I would vote for either circular approach or hard fail with a nicer exception

otherwise the one who will use something like that

sameResolution: "#{hello} #{hello} #{hello} #{hello}"

will spend lots of time for debugging

Maybe I don't understand, but current implementation do exactly that: fails with a clear exception:

java.lang.RuntimeException: Failed to invoke MethodAndCoercedArgs[method=DummyService.hello()...] ... Caused by: java.lang.IndexOutOfBoundsException: Index: 1 Size: 1

asolntsev marked this pull request as draft August 12, 2025 19:21

asolntsev added this to the 2.4.5 milestone Aug 12, 2025

asolntsev added the refactoring label Aug 12, 2025

asolntsev force-pushed the refactoring/base-faker-test branch from b8b0fc0 to b3b732d Compare August 12, 2025 19:24

asolntsev force-pushed the refactoring/base-faker-test branch from b3b732d to 4c07d4b Compare August 12, 2025 20:02

asolntsev added 6 commits August 12, 2025 23:04

remove extends BaseFakerTest from all test that don't need to run `…

ba1baf4

…testProviderList` and `testNoDuplications`. Also, force subclasses to implement `providerListTest()` by marking it as `abstract`. This simplifies understanding of what these tests verify.

fix typo in test name

ee6bbfe

remove method setFaker()

358c449

instead, just implement method `getFaker()`.

add missing providerListTest() for NationTest

0cafb7c

simplify implementation of LanguageCode.iso639()

f656d90

simplify mocking in tests

e42bc15

* don't use annotations `@Spy` and `@Mock` * don't need to reset mocks between tests Especially `FakeValuesServiceTest` got much simpler. It's not needed to mock so much. It's always better to use real objects instead of mocks.

asolntsev force-pushed the refactoring/base-faker-test branch from 4c07d4b to e42bc15 Compare August 12, 2025 20:04

asolntsev requested a review from snuyanzin August 12, 2025 20:23

asolntsev self-assigned this Aug 12, 2025

asolntsev marked this pull request as ready for review August 12, 2025 20:24

kingthorin requested a review from Copilot August 12, 2025 20:25

Copilot AI reviewed Aug 12, 2025

View reviewed changes

kingthorin approved these changes Aug 12, 2025

View reviewed changes

kingthorin merged commit 22730b5 into main Aug 12, 2025
13 checks passed

kingthorin deleted the refactoring/base-faker-test branch August 12, 2025 20:52

snuyanzin reviewed Aug 12, 2025

View reviewed changes

	private final Food food = getFaker().food();
	private final Food food = new Faker().food();


		import static org.assertj.core.api.Assertions.assertThat;

		final class HebrewFoodTest extends FoodFakerTest {

	private final FakeValuesService fakeValuesService = new FakeValuesService();
	private final FakeValuesService fakeValuesService = new FakeValuesService(context);

	DummyService dummy = new DummyService();
	DummyService dummy = new DummyService(null); // TODO: replace null with appropriate constructor arguments

	DummyService dummy = new DummyService();
	DummyService dummy = new DummyService(context);

Conversation

asolntsev commented Aug 12, 2025 • edited by kingthorin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

kingthorin commented Aug 12, 2025

Uh oh!

Uh oh!

snuyanzin Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Aug 12, 2025 • 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!

asolntsev Aug 13, 2025 • edited by kingthorin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kingthorin Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

asolntsev Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Aug 12, 2025 • 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!

asolntsev Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asolntsev commented Aug 12, 2025 •

edited by kingthorin

Loading

codecov-commenter commented Aug 12, 2025 •

edited

Loading

snuyanzin Aug 12, 2025 •

edited

Loading

asolntsev Aug 13, 2025 •

edited by kingthorin

Loading

snuyanzin Aug 12, 2025 •

edited

Loading