fix generation of phone numbers (domestic/international/mobile) #1225

asolntsev · 2024-05-27T22:11:32Z

I realized that PhoneNumberTest almost didn't test anything: it did 10 iterations, but checked that error count is < 25. :)

Now phone numbers are

fully correct for et, bg, mk, md, al, ru
partially correct for most countries (77% errors tolerance)
other countries are marked as FIXME.

codecov-commenter · 2024-05-27T22:13:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.10%. Comparing base (b37c566) to head (039d54e).
Report is 124 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    #1225      +/-   ##
============================================
- Coverage     92.35%   92.10%   -0.25%     
- Complexity     2821     3000     +179     
============================================
  Files           292      306      +14     
  Lines          5609     5906     +297     
  Branches        599      628      +29     
============================================
+ Hits           5180     5440     +260     
- Misses          275      306      +31     
- Partials        154      160       +6

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

I realized that PhoneNumberTest almost didn't test anything: it did 10 iterations, but checked that error count is < 25. :) Now phone numbers are * fully correct for et, bg, mk, md, al, ru * partially correct for most countries (77% errors tolerance) * other countries are marked as FIXME.

kingthorin · 2024-05-27T23:32:41Z

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

+        assertThat(phoneNumber.cellPhone()).matches("(044 )?\\(?\\d+\\)?([- .]\\d+){1,3}");
+        assertThat(phoneNumber.phoneNumber()).matches("\\(?\\d+\\)?([- .]\\d+){1,3}");


The groupings may as well be non-capturing where possible.

@kingthorin I agree, but now I feel out of resources to refactor these old regexp as well. :(

kingthorin · 2024-05-27T23:33:58Z

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

+    }
+
+    private static Arguments args(Locale locale, String phoneNumberRegion) {
+        return args(locale, phoneNumberRegion, (int) (COUNT * 0.77));


Thought you said 66?

may be also extract into a var to be able to change everything at one place in case of need?

Thought you said 66?

Yes, initially I put 66%, but later had to increase this number to 77% because of flaky tests failed on CI. :)

extract into a var

Yes, I've extracted 77% to a constant.

snuyanzin · 2024-05-28T05:48:35Z

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

+    private static Arguments args(Locale locale, String phoneNumberRegion, int allowedErrorsCount) {
+        return Arguments.of(locale, phoneNumberRegion, allowedErrorsCount);


should we just import static for Arguments and then it could be shorter like

of(locale, phoneNumberRegion, allowedErrorsCount);

?

It's shorter, but is it really readable?
When I read just "of(xxx)", how can I understand what this "of" means"? Of what?

Compared to that, Arguments.of is more readable.

I guess it depends on the context
if we are talking about of in general then I would agree it is questionable, however here we are in test class, in arguments provider method, with this knowledge it is more readable then from my point of view

anyway i'm not going to stop merging just because of this
I expressed my concerns and leave the final decision up to you

I'm a huge fan of static imports (AssertJ for example), but in this case, importing "of" doesn't really add to readability, so I'd lean towards keeping the classname there

Oh, I'm too late it seems :)

snuyanzin · 2024-05-28T06:12:14Z

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

+                if (log.isLoggable(FINE)) {
+                    log.fine("Invalid phone: %s".formatted(phoneNumber));
+                }
+                errorCount++;


Suggested change

errorCount++;

errorCount++;

if (allowedErrorsCount > errorCount) {

break;

}

I guess there is no need to iterate if we already exceed the limit

+ may be similar approach when a number of rest iterations is less than amount of error we still could make and pass the tests... however in this case there will be less log output 🤔

Yep, I've added "break" at these places.
Though, this code is temporary anyway. We need to configure phone numbers to achieve 0 error tolerance. :)

I think I tried that some time ago, to get close to zero tolerance, but while I managed for some countries, it was non trivial to get them all in shape. Happy if you give it a shot though

asolntsev self-assigned this May 27, 2024

kingthorin reviewed May 27, 2024

View reviewed changes

snuyanzin reviewed May 28, 2024

View reviewed changes

asolntsev added 2 commits May 28, 2024 09:40

extract the magic number 77% to a constant

c4eb70b

stop the loop sooner if errors count is already too big

039d54e

snuyanzin approved these changes May 28, 2024

View reviewed changes

asolntsev merged commit efc9270 into datafaker-net:main May 28, 2024

asolntsev deleted the fix/phone-numbers branch May 28, 2024 10:12

		assertThat(phoneNumber.cellPhone()).matches("(044 )?\\(?\\d+\\)?([- .]\\d+){1,3}");
		assertThat(phoneNumber.phoneNumber()).matches("\\(?\\d+\\)?([- .]\\d+){1,3}");

		private static Arguments args(Locale locale, String phoneNumberRegion, int allowedErrorsCount) {
		return Arguments.of(locale, phoneNumberRegion, allowedErrorsCount);

fix generation of phone numbers (domestic/international/mobile) #1225

fix generation of phone numbers (domestic/international/mobile) #1225

Uh oh!

Conversation

asolntsev commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

snuyanzin May 28, 2024 • 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!

asolntsev May 28, 2024 • 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!

snuyanzin May 28, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asolntsev commented May 27, 2024 •

edited

Loading

codecov-commenter commented May 27, 2024 •

edited

Loading

snuyanzin May 28, 2024 •

edited

Loading

asolntsev May 28, 2024 •

edited

Loading

snuyanzin May 28, 2024 •

edited

Loading