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

Added test coverage for DatabaseOperations.sql_flush(). #12729

Merged
merged 1 commit into from Apr 17, 2020
Merged

Added test coverage for DatabaseOperations.sql_flush(). #12729

merged 1 commit into from Apr 17, 2020

Conversation

jdufresne
Copy link
Member

No description provided.

@charettes
Copy link
Member

charettes commented Apr 16, 2020

I wonder if it would be better to write a backend agnostic test that executes the provided SQL and ensures tables are flushed and sequences are reset. I'm not sure there's a lot of values in asserting against the exact SQL generated since we offer no stability guarantees in this regard.

@jdufresne
Copy link
Member Author

jdufresne commented Apr 16, 2020

I wonder if it would be better to write a backend agnostic test that executes the provided SQL and ensures tables are flushed and sequences are reset.

I can work towards that.

I'm not sure there's a lot of values in asserting against the exact SQL generated since we offer no stability guarantees in this regard.

I think it provides value. It shows exactly (modulo some ordering) the output of the function. We can inspect this output directly to ensure the commands that we expect to render, are returned. A change that unexpectedly changes these statements -- perhaps causing a regression -- will be noticed.

One of the motivations of this PR is that I would like to change PostgreSQL sql_flush() to change the sequence resetting logic from multiple SQL statements to use the RESTART IDENTITY keyword of the TRUNCATE statement. My project uses flush operation internally and some profiling shows that it can consume a large percentage of the runtime. As these functions have minimal test coverage so far, this test adds a minimal base level of assurance before any changes take place. Asserting the function's exact return value will show me that my upcoming change to use RESTART IDENTITY will be implemented.

I do agree there is additional value in executing the SQL as well. I think they would complement one another.

@charettes
Copy link
Member

charettes commented Apr 16, 2020

change the sequence resetting logic from multiple SQL statements to use the RESTART IDENTITY keyword of the TRUNCATE statement.

Oh I looked at it as well while helping with the MySQL adjustments that took place in this area but I assumed it was only working with identity columns and thus was blocked by #30511. I must have misread the PostgreSQL documentation since it clearly state it will reset sequences as well.

I don't see much harm in testing the exact SQL as well in this case.

@jdufresne
Copy link
Member Author

write a backend agnostic test that executes the provided SQL and ensures tables are flushed and sequences are reset.

👍 I added ExecuteSqlFlushTests for this.

@felixxm felixxm changed the title Added test coverage for backends' sql_flush() operation. Added test coverage for DatabaseOperations.sql_flush() operation. Apr 17, 2020
@felixxm felixxm changed the title Added test coverage for DatabaseOperations.sql_flush() operation. Added test coverage for DatabaseOperations.sql_flush(). Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants