Skip to content

Make FakerRepeatabilityIntegrationTest running in same thread #837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 16, 2023

Conversation

snuyanzin
Copy link
Collaborator

No description provided.

@kingthorin
Copy link
Collaborator

Is this instead of the other PR or in addition?

@snuyanzin
Copy link
Collaborator Author

addition to fix repeatability

@bodiam
Copy link
Contributor

bodiam commented May 15, 2023

I don't see a reason for this. This code worked fine before d9c2cc262d29deb3552c9f49ab71edf0b3f6dc30.

This seems to hide that we still have a concurrency issue.

@snuyanzin
Copy link
Collaborator Author

how do you that exactly d9c2cc2 is the reason?
after that there were about 10 more commits to main branch and no such issue

@bodiam
Copy link
Contributor

bodiam commented May 15, 2023

how do you that exactly d9c2cc2 is the reason? after that there were about 10 more commits to main branch and no such issue

Because I checked out all these commits:

image

And all of them work consistently. Except for d9c2cc262d29deb3552c9f49ab71edf0b3f6dc30. If I added a repeatedtest on the method there, things start failing.

d9c2cc262d29deb3552c9f49ab71edf0b3f6dc30: fails
b840846fcfbf88112241b398faced21903435232: works
1e7544bd78a11563bed22c25af91f550a90d52ed: works
9a32f4b0933c74f3e1298a6a36341a6301b2e09f: works

update:

6ee07320144dbba7ddf06e1fbd62db4ce6bd8a4c: works
64e52940983202fbb0387044491ef8178ac46bd6: works
61eb73fa4a7bc5ae718b582b69d4d998c2be1ea6: works
a3f9995b21ee928bd17c828720a16817aad5b9fd: works
88c0e389a27775f8aa90df6c7a6e79051af7d851: works
b2cb6a803a46e7a82535cfb3add607098f62439d: FAIL

In the main branch, things started failing since the Azure Spring Apps are there??

@bodiam
Copy link
Contributor

bodiam commented May 15, 2023

So, that's interesting: if we enabled the springApps() method in Azure, things started failing.

@snuyanzin
Copy link
Collaborator Author

i wonder if it's because of springApps() or just because of any 1 more method which means that if we still have enabled springApps() however remove any another method will it work?

@bodiam
Copy link
Contributor

bodiam commented May 15, 2023

i wonder if it's because of springApps() or just because of any 1 more method which means that if we still have enabled springApps() however remove any another method will it work?

I don't think it's because of springApps per se, it's not doing much, but it seems that just calling the randHex method is triggering the issue.

I've done a few random tests, and the funny things is:

    public String springApps() {
        String hex = faker.random().hex(4, false); // works
        return "sa-" + hex;
    }

This consistently works.

    public String springApps() {
        String hex = faker.random().hex(8, false); // works
        return "sa-" + hex;
    }

but this fails:

    public String springApps() {
        String hex = faker.random().hex(9, false); // 9 fails. Anything above fails. 8 = 1 byte, 9 = 2 bytes?
        return "sa-" + hex;
    }

But I don't understand it. All the other methods are using the same code, but with 16, yet they don't fail, but this does. I'm probably chasing ghosts here?

@snuyanzin
Copy link
Collaborator Author

After some tests it seems springApps is only a trigger and not the rootcause. The reason is CopyOnWriteMap introduced in #770
Where in case of high throughput some values could be removed and recalculated once again (normal behavior of copy on write)

well I still suggest to make repeatabale tests in same thread.

Before that fix we anyway didn't support multithreading for FakeValuesService as mentioned at #759

@bodiam
Copy link
Contributor

bodiam commented May 15, 2023

well I still suggest to make repeatabale tests in same thread.

Okay, that's fine!

@snuyanzin snuyanzin merged commit e77c267 into datafaker-net:main May 16, 2023
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.

3 participants