Skip to content

[make:crud][experimental] Add generated tests to make:crud #307

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 20 commits into from
May 9, 2022

Conversation

ckrack
Copy link
Contributor

@ckrack ckrack commented Oct 26, 2018

Add tests for the controller methods.
Add necessary dependencies.
Closes #229

Edit by @jrushlow:

  • Feature is experimental - We generate a minimum WebTestCase for generated CRUD.
  • Generating tests is on an opt-in basis if the user has test-pack already installed. Otherwise we're not advertising this feature yet.
  • Entity / Form Field guessing is not fully implemented. "Setter" values other than string will not match the Entity's property types and must be manually changed.
  • Tests (other than testIndex()) are all generated with markTestIncomplete()
  • Better integration w/ Panther, Fixtures, Foundry, etc... to come in future PR's

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @ckrack! That's my bad - but I'm pretty excited about the possibilities with this PR. Let's chat if you have questions / thoughts.


// /**
// * @TODO implement edit test
// * @depends testNew
Copy link
Member

Choose a reason for hiding this comment

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

Instead of depending on testNew, we should create an entity directly via the EntityManager here and persist it. Depending on testNew is convenient at first, but it couples the test methods together, which has a practical downside: I now need to run two full tests before running this test (testIndex, then testNew, then this one).

Btw, I'm not so sure that we need to comment out all of the edit tests. Because... we know what entity is being generated. And so, we could write some code to instantiate that object and persist it. Heck... thinking crazy, we could use faker and introspect the Doctrine annotation information to even fill in the required fields if they have Faker installed (we could recommend it).

It's a bit more work, but in theory, we could make these tests fully (or nearly fully) functional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have looked into using Faker for the test generation.

What is easily achievable is using Faker inside the tests to populate Entities based on their properties.
However, this will lead to tests never failing when properties were changed. Faker will always use an up-to-date generator, because the guesser bases on the current entity
Tests will only fail if the Form is not updated and requires a change, too.

This looks like:

// App\Test\Controller\ExampleControllerTest;
// inside a test* method that needs an entity in the db:
$entityPopulator = new EntityPopulator($this->entityManager->getClassMetadata(Test::class));
$entityPopulator->setColumnFormatters($entityPopulator->guessColumnFormatters($this->fakerGenerator));

/** @var App\Entity\Test */
$test = $entityPopulator->execute($this->entityManager, []);

$this->entityManager->persist($test);
$this->entityManager->flush();

One could argument that this is enough, because we are testing the controller - not the entity or form.
I'd prefer to generate tests that contain somewhat fixed values and will fail, when either the entity or form are changed.
So tests could either contain faker generator calls $faker->name() or the output of this. The former would require reverse-engineering Faker's name guesser, as the populators are returned as closures and do not allow retrieving their name.
This means we must keep a copy of this up to date, to allow getting the generator names as string: https://github.com/fzaninotto/Faker/blob/master/src/Faker/Guesser/Name.php
Not the best option.

This is also complicated, due to association mappings.
We can leave them out or auto-populate them in the tests.
If we leave them out, the generated tests will fail - our test of the maker command must reflect this.

WDYT?

@weaverryan weaverryan added Help Wanted Status: Needs Work Additional work is needed labels Jun 14, 2019
@weaverryan
Copy link
Member

@ckrack Do you still have an interest in finishing this? Or should we try to find another volunteer? :)

@TheGarious
Copy link

Up.

Need Help for finish this PR ?

@weaverryan weaverryan changed the base branch from master to main November 16, 2020 20:03
@jrushlow jrushlow added Feature New Feature and removed Help Wanted labels May 9, 2022
@jrushlow jrushlow self-assigned this May 9, 2022
@jrushlow jrushlow changed the title [CRUD] Add generated tests to make:crud [make:crud] Add generated tests to make:crud May 9, 2022
@jrushlow jrushlow removed the Status: Needs Work Additional work is needed label May 9, 2022
@jrushlow jrushlow added the Status: Needs Review Needs to be reviewed label May 9, 2022
@jrushlow jrushlow changed the title [make:crud] Add generated tests to make:crud [make:crud][experimental] Add generated tests to make:crud May 9, 2022

if (class_exists(CssSelectorConverter::class)) {
$this->generateTests = $io->confirm('Do you want to generate tests for the controller? [Experimental]', false);
}
Copy link
Member

Choose a reason for hiding this comment

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

What would happen if we didn't check for the class? And just always asked the question? If they ran the tests, obviously they wouldn't work, but would it be obvious that they just need to install some stuff? I'm trying to limit the number of conditionals and paths we have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Welp nothing - good catch!

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Thanks @ckrack for getting this going!

@weaverryan
Copy link
Member

Thank you @ckrack and @jrushlow!

@weaverryan weaverryan merged commit 767be43 into symfony:main May 9, 2022
@jrushlow jrushlow mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make:crud to make basic tests for each route
4 participants