faif / python-patterns

A collection of design patterns/idioms in Python
40.2k stars 6.93k forks source link

Abstract factory creational methods doctests fail #418

Closed grimley517 closed 3 months ago

grimley517 commented 4 months ago

Context

Arrising from PR #417 and issue #415

The doctests for the creation abstract factory are not working. code reference

The issue here appears to be that the random seed isn't making the random selection of pet store determinative.

Reproduction Steps

Run the tests using the lint and test script. (lint.sh in PR #417), or via the github pipeline

Expected Result

Tests pass

Actual result

Doctests fail, with animals assigned randomly

Solutions

The code appears to be good, this is the tests failing. The aim of the code is to demonstrate the pattern. The solution should be to strengthen the tests.

Remove the random pet shop

Since the code's aim is to demonstrate the abstract creation pattern, the random petshop may not be the most testable way to do this. My preference is to remove the random animal petshop from this set of tests. There are some options on what to replace it with in order to demonstrate the pattern.

Refactor the tests

Doctests are not sufficiently flexible to handle the non-determinative nature of the random pet shop. The answer could be to remove them as doctests and replace them with traditional unit tests, which demonstrate that the type of animal depends on the kind of pet sold.

Conclusion

I prefer to remove the random pet shop altogether or replace it with an alternating pet shop. The question that should differentiate between these two approaches is whether the test adds anything to the explanation.

faif commented 4 months ago

I thought that a contributor added a fixed seed to avoid the randomness issue, so not sure when that broke. I agree with you, the random part is not necessary so we can remove it

grimley517 commented 3 months ago

This issue is resolved with #415