Skip to content
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

Extract fixtures into separate classes #234

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

localheinz
Copy link
Member

@localheinz localheinz commented Dec 22, 2020

This PR

  • extracts fixtures into separate classes and introduces a new PSR-4 class mapping

πŸ’β€β™‚οΈ There is a range of test classes in tests/Faker. Some of the files declare additional classes in the containing files.

The first impulse would be to move them into separate files, declaring a single class in each file. The question that arises then is: where to put these classes?

We use Faker\Test as namespace for tests. We could

  • declare test doubles in Faker\Test\Double
  • declare fixtures in Faker\Test\Fixture
  • declare integration tests in Faker\Test\Integration
  • declare unit tests in Faker\Test\Unit

This could nicely map to the following directory structure:

test
β”œβ”€β”€ Double
β”œβ”€β”€ Fixture
β”œβ”€β”€ Integration
└── Unit

However, we currently autoload classes from the Faker\Test namespace using a PSR-4 autoloader with PSR-0 class mapping with the following directory structure:

test
└── Faker

We could move classes into the following structure:

test
└── Faker
 Β Β  β”œβ”€β”€ Double
 Β Β  β”œβ”€β”€ Fixture
 Β Β  β”œβ”€β”€ Integration
 Β Β  └── Unit

but then these classes would mix with existing classes.

We could move existing classes from test/Faker/ to test/, effectively flattening the directory structure. People often complain that this will make history hard to understand or follow (I don't care much). Since a lot of classes will be removed or extracted into separate packages, these tests will disappear sooner or later, too.

That's why I propose to keep the old tests and add new tests in a separate structure:

test
β”œβ”€β”€ Double
β”œβ”€β”€ Faker
β”œβ”€β”€ Fixture
β”œβ”€β”€ Integration
└── Unit

We could then easily differentiate between kinds of tests, in particular, tests that demonstrate the behaviour from the outside as expected from users (also see #233), and others, that test specifics of extensions and similar.

Eventually, we could remove the directory test/Faker.

What do you think?

@localheinz localheinz added the enhancement New feature or request label Dec 22, 2020
@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #234 (0d4b706) into main (3d69b4d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #234   +/-   ##
=========================================
  Coverage     58.07%   58.07%           
  Complexity     2044     2044           
=========================================
  Files           315      315           
  Lines          5104     5104           
=========================================
  Hits           2964     2964           
  Misses         2140     2140           

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 3d69b4d...86ebdd0. Read the comment docs.

@bram-pkg bram-pkg changed the title Enhancement: Extract fixtures into separate classes Extract fixtures into separate classes Jan 5, 2021
@Nyholm
Copy link
Member

Nyholm commented Jan 7, 2021

I like this PR. Does it make sense to leave all/most of the current code as is, and then when we move to extensions, we just copy the tests we like. Eventually we remove the legacy tests.

@pimjansen
Copy link

Yeah agree on the approach

@pimjansen
Copy link

@localheinz will you make a first setup of this or should some of us support you with anything here?

@stale
Copy link

stale bot commented Jan 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 1 week if no further activity occurs. Thank you for your contributions.

@localheinz
Copy link
Member Author

@pimjansen

Needs a review!

πŸ€“

@stale stale bot removed the lifecycle/stale label Jan 26, 2021
@GrahamCampbell
Copy link
Member

GrahamCampbell commented Jan 26, 2021

Looks fine. Advantage is that we are actually following psr-4 now... :P

@pimjansen
Copy link

Lgtm

@localheinz localheinz merged commit 619ceca into FakerPHP:main Jan 28, 2021
@localheinz localheinz deleted the feature/fixture branch January 28, 2021 12:08
@localheinz
Copy link
Member Author

Thank you, @GrahamCampbell, @Nyholm, and @pimjansen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pinned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants