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

Make creating test models more straightforward and revert to swallowing exceptions #9526

Merged
merged 1 commit into from
Feb 20, 2022

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Feb 19, 2022

In #8962, I established that
swallowing exceptions while creating model was bad because it could hide
other helpful exceptions.
As it turns out however, swallowing exceptions is really the way to go
here, because of the performance implication of calling dropSchema(). It
is possible to catch a more precise exception as well, which should
preserve the benefits of not swallowing them.

It looks like I based my grep on the comment inside the catch and today,
I found many other occurrences of this pattern, without the easy to grep
comment.

I decided to fix them as well, but in a lazier way: one no longer has to
remember to call dropSchema in tearDown() now, it is automated.

Here is the grep I used this time: rg --multiline '\} catch \(Exception \$e\) \{\n\s+\}'

TODO

@greg0ire
Copy link
Member Author

greg0ire commented Feb 19, 2022

@derrabus I'm requesting your review now because it's the most legible and interesting part.

EDIT: less so now that I have completed the first item of that todo list

@greg0ire greg0ire force-pushed the better-model-setup branch 4 times, most recently from a173b08 to 74cefd7 Compare February 19, 2022 11:15
@beberlei
Copy link
Member

DropSchema might be a bad idea, because schema creation is so expensive except for sqlite

@greg0ire
Copy link
Member Author

greg0ire commented Feb 19, 2022

@beberlei I didn't check that in #8962

Once this is completed, I can rework createSchemaForModels to use exception swallowing and drop the @after method, that way we will be able to measure the difference.

EDIT: oh now it's failing because some tests create cms_user without dropping it 😞 I will have to bisect this.

@greg0ire greg0ire force-pushed the better-model-setup branch 3 times, most recently from 75be1a0 to fbbb858 Compare February 19, 2022 11:38
@greg0ire
Copy link
Member Author

@beberlei it looks like the performance loss is so big I can even "feel" it, even with SQLite. Let's revert to swallowing exceptions. I'll use a more specific exception, which should fix the issue I was aiming to fix in #8962 (I remember getting mad because the test was hiding an exception from me, which was probably from a different hierarchy that the one we want to catch).

@greg0ire greg0ire changed the title Make creating test models more straightforward Make creating test models more straightforward and revert to swallowing exceptions Feb 19, 2022
@greg0ire greg0ire force-pushed the better-model-setup branch 2 times, most recently from 9307dac to 6a1657a Compare February 19, 2022 18:55
@greg0ire greg0ire marked this pull request as ready for review February 19, 2022 18:56
@greg0ire greg0ire force-pushed the better-model-setup branch 2 times, most recently from 15b685e to 0261a15 Compare February 19, 2022 19:04
@greg0ire
Copy link
Member Author

@beberlei now that I reverted what I did in #8962: there seems to be a big performance gain (or rather, I introduce a big performance loss before 😞 ) :

@derrabus
Copy link
Member

LGTM, once the doc block is fixed. The code is much more readable this way, very nice.

In doctrine#8962, I established that
swallowing exceptions while creating model was bad because it could hide
other helpful exceptions.
As it turns out however, swallowing exceptions is really the way to go
here, because of the performance implication of calling dropSchema(). It
is possible to catch a more precise exception as well, which should
preserve the benefits of not swallowing them.

It looks like I based my grep on the comment inside the catch and today,
I found many other occurrences of this pattern, without the easy to grep
comment.

I decided to fix them as well, but in a lazier way: one no longer has to
remember to call dropSchema in tearDown() now, it is automated.
@greg0ire greg0ire merged commit 5b8263e into doctrine:2.11.x Feb 20, 2022
@greg0ire greg0ire deleted the better-model-setup branch February 20, 2022 08:35
@greg0ire greg0ire added this to the 2.11.2 milestone Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants