Skip to content

show actual values in error message#1196

Merged
snuyanzin merged 5 commits into
datafaker-net:mainfrom
asolntsev:improve-test
May 18, 2024
Merged

show actual values in error message#1196
snuyanzin merged 5 commits into
datafaker-net:mainfrom
asolntsev:improve-test

Conversation

@asolntsev
Copy link
Copy Markdown
Collaborator

@asolntsev asolntsev commented May 18, 2024

... instead of just throwing an empty runtime exception.

Problem

Before this change, test might fail with an unclear message:

java.lang.RuntimeException
	at net.datafaker.Issue759.issue759Test(Issue759.java:93)

or the output of multiple failing tests might be mixed:

 0 0 0 0 0
 0 0 0 0 0
 0 0 0 0 0 0
 0 0 0 0 0 0 0 0 0 0 0 0 0 0
 0 0 0 0 0
 0 0 0
 0 0 0
 0 0 0
 0

 0 0 0 0 0

java.lang.RuntimeException
	at net.datafaker.Issue759.issue759Test(Issue759.java:93)

Solution

Now the error message is clear:

java.lang.AssertionError: Not all of s[0, 0, 0, 0, 0] are not equal to 20000

	at net.datafaker.Issue759Test.issue759Test(Issue759Test.java:85)

P.S.

To reproduce the issue, just put breakpoint, say, to any field of WorkerThread, run Issue759 test in debug mode and resume the execution after stopping on the breakpoint.

... instead of just throwing an empty runtime exception
Comment thread src/test/java/net/datafaker/Issue759Test.java Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.10%. Comparing base (b37c566) to head (e973bce).
Report is 97 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    #1196      +/-   ##
============================================
- Coverage     92.35%   92.10%   -0.25%     
- Complexity     2821     2891      +70     
============================================
  Files           292      300       +8     
  Lines          5609     5726     +117     
  Branches        599      618      +19     
============================================
+ Hits           5180     5274      +94     
- Misses          275      293      +18     
- Partials        154      159       +5     

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

asolntsev and others added 3 commits May 18, 2024 10:56
One "s" and one "not" were not needed.

The intended variant was:

`Not all of [1,2,3,4,3,6,3,7] are equal to 3.`
@snuyanzin
Copy link
Copy Markdown
Collaborator

snuyanzin commented May 18, 2024

Thanks for your contribution @asolntsev

it seems there was a long standing issue with this test, which apparently started to reproduce with your PR [1] on Windows only...
I submitted a fix for that (a bit more description is below)
If you are ok with it, we can merge it, otherwise please express your concerns

About the issue:
It looks the problem was with the code

        int[] lastIters = getIterations(threads);
        while (true) {
            Thread.sleep(100);
            int[] iters = getIterations(threads);
            if (Arrays.equals(lastIters, iters)) {
                // Either all threads are done, or something is probably stuck
                System.err.println();
                if (allElementsEqual(iters, iterationsPerThread)) {
                    break;
                } else {
                    printIterations(iters);
                    throw new RuntimeException();
                }
            }

On Windows it could happen that we reach the line if (allElementsEqual(iters, iterationsPerThread)) { before the threads are even started... as a result there was none iterations executed...
The idea of fix is to measure amount of time of being unchanged and react on the fact that this amount of time is exceeded some limit.

[1] https://github.com/datafaker-net/datafaker/actions/runs/9138381122/job/25130594393#step:4:792

@asolntsev
Copy link
Copy Markdown
Collaborator Author

@snuyanzin Thank you for sharing the information.
Now I looked at the test once more and realized it can be simplified using Java built-in class CountDownLatch.
Look my last commit. :)

void issue759Test() throws InterruptedException {
int numThreads = 5;
int iterationsPerThread = 20000;
CountDownLatch countDownLatch = new CountDownLatch(numThreads * 20000);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i guess we could reuse the second var as well

Suggested change
CountDownLatch countDownLatch = new CountDownLatch(numThreads * 20000);
CountDownLatch countDownLatch = new CountDownLatch(numThreads * iterationsPerThread);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thank you. Fixed. @snuyanzin

@snuyanzin
Copy link
Copy Markdown
Collaborator

@asolntsev Indeed it is better approach
Thanks a lot!
It look great

I left one tiny comment

the goal of this test of to verify that Faker doesn't fail in multi-threaded environment. So it's enough to just check that all steps has been executed without any failures.

1. CountDownLatch initially contains total steps count,
2. Every test step decreases this by one,
3. And the test just waits until the counter becomes 0.
@snuyanzin snuyanzin merged commit 91aaac7 into datafaker-net:main May 18, 2024
@asolntsev asolntsev deleted the improve-test branch May 18, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants