[graphql-fixtures] Remove unused call to faker.seed()

[notjosh]

Description

Hi! We're seeing lots of calls to faker.seed() when using this library, and it turns out (predictably) that's because we call it a lot in some situations! 😱

The problem here, is that init_genrand() is quite an expensive operation (source). It was showing to be using 2-4% of time spent in test suites!

As a starting point, this PR cuts the calls to init_genrand() to around 50%, without any noticeable negative side effect.

Discussion: For future, we could replace the Mersenne Twister implementation for less cryptographically secure but cheaper + good enough for our needs. For example, an implementation of an LCG with an essentially free seeding function. I have a patch for this that I used in profiling to swap out the implementation in Faker.js, but a) it's obviously fragile to patch a core function like that, and b) it will break existing tests relying on specific seeded output.

(This ventures deep into "should we even still use Faker?" territory though, but it's food for thought)

Type of change

• graphql-fixtures Patch: Bug (non-breaking change which fixes an issue)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

[atesgoral]

should we even still use Faker?

My first instinct is, no.

I don't see why we want unpredictable/nondeterministic test fixtures.

[atesgoral]

We're seeing

Which repo was it?

I wonder what happens to Shopify/web tests with this patch.

[notjosh]

Which repo was it?

Shopify/arrive-client. Tests remain green for that suite with the patch, but I'd also be curious if there are any side effects on other repos.

[notjosh]

I don't see why we want unpredictable/nondeterministic test fixtures.

That's what the calls to faker.seed() prevent - the fixtures are deterministic based on the key/property (afaict). It gives repeatable & readable data, which is nice, so I can see the benefit. But it's not built for speed! (see also #2152)

[atesgoral]

What I implicitly (or in my own head) meant was: Even with seeding, tests might be prone to breaking the moment there's a slight tweak to how the PRNG or data synthesis works. I would hard-code the input and outputs in tests.

Like in this very particular case, where there's a performance win opportunity, but we can't be sure about the side effects 😓

We could try yalc'ing this in Shopify/web or do a beta release, maybe.

[notjosh]

What I implicitly (or in my own head) meant was: Even with seeding, tests might be prone to breaking the moment there's a slight tweak to how the PRNG or data synthesis works. I would hard-code the input and outputs in tests.

Ohhh right, yes. If we change the PRNG, tests will (and do!) definitely break, so I haven't touched that. This change doesn't change any of the output used for fixtures at all, and gets re-seeded with a deterministic seed every time it's called.

I'd imagine the only way this could break tests would be extremely specific, i.e. tests that a) mock Math.random() for some reason (maybe to imitate seeding the value), and b) rely specifically on the instance of Faker set up with this module (which would be left with a different seed after execution).

And for completeness: other libs pulling in Faker wouldn't be impacted, as they get (re-)seeded separately in their own instance.

[BPScott]

@ryanwilsonperkin you've been knee deep in faker seeding funkyness. Got any thoughts?

[ryanwilsonperkin]

Copying a couple comments over from slack:

This might break the odd thing in web, but probably not too to much because we already came across and fixed a few of the places where we were asserting on specific values that we expected to come back from the seed.

We’d still need some determinism within a given test though, I’m not sure we can remove all the calls to seed. Simplest example to demonstrate would be if you had multiple useQuery calls within the same component and you expected them to each resolve (potentially nested) a Widget type. If you don’t provide an id value in your partial fill argument, then you’ll get a deterministic id value for the Widget in each of the query responses. If we remove the seeding, then this would result in each one getting a different id, which then breaks Apollo (it returns undefined)

[notjosh]

^Totally agreed, but this is only removing one of the calls to seed which uses Math.random() as the seed (so not deterministic anyway), so the withRandom method will still be deterministic in the same way it was before

[ryanwilsonperkin]

Ran a subset of web tests using this branch through yalc and seems to be working fine, couldn't run the full test suite locally but this gives us some confidence that this won't break any paths we were worried about. LGTM 👍🏻

Please update the changelog with a note on the changed functionality, and we can release a new patch version to do a full test on web.

[BPScott]

CI failures due to shifting node versions fixed in #2171. Rebase and CI should be happy again