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

Add ability to clean the DB #15827

Closed

Conversation

stuartwdouglas
Copy link
Member

@stuartwdouglas stuartwdouglas commented Mar 18, 2021

  • In tests this is done via @ResetDatabase
  • In dev mode this is done from the dev console

Fixes #583

@yrodiere
Copy link
Member

This feature looks nice! I have two questions:

  • Shouldn't this reset the database before the test? If I annotate a given test method, I'm mostly interested in what happens during this particular test...
  • Could this be adapted to reset persisted data that is not strictly speaking in a database, e.g. distributed caches, MongoDB, Elasticsearch, ...? And if so, shouldn't the name be more generic, such as @ResetPersistence? @ResetData, maybe?

@famod
Copy link
Member

famod commented Mar 18, 2021

Nice!

Are you aware of #14240?
I wonder if that DatabaseSchemaProvider approach could be extended/used to allow a DB dump/restore with DB-specific tools like outlined in #14240 (comment)
The scenario would be that you have quarkus-liquibase in the project and if the DB is blank in the beginning, Liquibase shall be executed, but everything after that should use a much quicker dump/restore mechanism...

for (DatabaseSchemaProvider i : dbs) {
i.resetDatabase(name);
}
flashMessage(event, "Action invoked");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
flashMessage(event, "Action invoked");
flashMessage(event, "Database "+name+" reset");

//generate the annotated interceptor with gizmo
//all the logic is in the parent, but we don't have access to the
//binding annotation here
try (ClassCreator c = ClassCreator.builder()
Copy link
Member

Choose a reason for hiding this comment

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

The shit we do to deal with modularity :(

RestAssured.when().get("/my-entity/count").then().body(is("2"));
RestAssured.when().get("/my-entity/add").then().body(is("MyEntity:added"));
RestAssured.when().get("/my-entity/count").then().body(is("3"));
RestAssured.with()
Copy link
Member

Choose a reason for hiding this comment

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

I never realised that tests could access dev mode actions.

@FroMage
Copy link
Member

FroMage commented Mar 18, 2021

That PR is awesome!

I do agree with @yrodiere that I was expecting the DB to be cleaned before my tests. And also that a similar mechanism could be added for other things like caches, but I guess that this can wait until later.

@Sanne
Copy link
Member

Sanne commented Mar 18, 2021

nice!

We'll have to be careful to allow us to be smart under the hood though, as some DBs are painfully slow to deal with schema changes; just yesterday on the ORM zulip channels we were talking about a (TBD) feature which could drop the data only rather than removing and re-creating the schema.

@stuartwdouglas
Copy link
Member Author

I would expect tests to leave the DB in a consistent state by default, so there should be no need to clean up beforehand. This annotation is basically saying 'I am going to leave the DB in an inconsistent state, I want you to clean it up for me'

- In tests this is done via @CleanDatabase
- In dev mode this is done from the dev console

Fixes quarkusio#583
@famod
Copy link
Member

famod commented Mar 18, 2021

Sometimes, it can be handy to check in the DB what the test has left behind without having to debug it.
Temporarily removing the annotation leaves you with a dirty state for the next run, so that's not an option.
Maybe an annotation parameter could control when the cleanup happens?

@yrodiere
Copy link
Member

I would expect tests to leave the DB in a consistent state by default, so there should be no need to clean up beforehand. This annotation is basically saying 'I am going to leave the DB in an inconsistent state, I want you to clean it up for me'

If we assume that this annotation is only useful within the scope of a single test class, because every test class always resets the database(s) before executing, then yes I agree.

Otherwise, I'm afraid that's wishful thinking: people will end up in a situation where the DB is in an inconsistent state eventually. Be it because the application crashed during the last test execution (OOM, stackoverflow), because you stopped the JVM while debugging, ...

Also... what do you think about changing the name to allow it to cover non-DB cleanups in the future? @ResetData, @ResetPersistence, whatever works?

@FroMage
Copy link
Member

FroMage commented Mar 19, 2021

I would expect tests to leave the DB in a consistent state by default, so there should be no need to clean up beforehand. This annotation is basically saying 'I am going to leave the DB in an inconsistent state, I want you to clean it up for me'

That's optimistic at best ;)

In practice I always have some tests that don't give a damn about the initial state of the DB, so they can all run without any cleanup. And then some really care and need a clean DB before them.

It's true that I could fix both tests by cleaning up after them. BTW, is there a way to tell a test class to cleanup after all tests versus after each test?

@stuartwdouglas
Copy link
Member Author

Because this is an interceptor I can't really do 'after the class', only 'after each method', it would need a different implementation if we were going to support the 'after class' approach.

I have also been thinking about native tests, and what we can do to support it. It's a lot more complex, as you don't really want a way for the test to trigger a reset of the DB (as this could be a huge security issue, if there is a way to tell a production instance to clean the production DB).

@stuartwdouglas
Copy link
Member Author

I have opened #19312 for just the Dev UI part

@stuartwdouglas
Copy link
Member Author

Closing this, as support for added to the dev console

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.

Consider offering a way to reset DB at Hibernate ORM level for testing
5 participants