Support runWith executors committing after the change set completes #5329
Conversation
|
Hi @filipelautert , |
filipelautert
force-pushed
the
master
branch
from
5ec467a
to
743d38a
Compare
filipelautert
temporarily deployed
to
external
GitHub Actions
Inactive
|
Hello @dzeigler , thanks for the PR! |
filipelautert
force-pushed
the
master
branch
from
743d38a
to
a30555a
Compare
filipelautert
had a problem deploying
to
external
GitHub Actions
Failure
filipelautert
force-pushed
the
master
branch
from
a30555a
to
b6078ff
Compare
|
Hi @dzeigler, I've been looking at your code changes and they look good to me. Something I was thinking it would be nice to have is another test scenario where we have a changeset with multiple changes, so we make sure after executing each of them executor triggers a commit and then we verify changes have been successfully deployed in the DB. This sounds to me more like an integration test than a unit test. Can we do that? Thanks, |
|
I agree. I thought about adding coverage for that scenario but wasn't sure if that was appropriate. I'll look at what's involved in adding the integration test and update accordingly sometime in the next few days. |
Nice, thank you @dzeigler! |
dzeigler
force-pushed
the
master
branch
2 times, most recently
from
4837807
to
2fd1550
Compare
|
@MalloD12 , please check my most recent commit. Its commit message explains the approach I took for the test. Let me know if anything needs to be adjusted. |
liquibase-integration-tests/src/test/java/liquibase/dbtest/AbstractIntegrationTest.java
Outdated
Show resolved
Hide resolved
…r's execute(Change, List<SqlVisitor>) method. Delegating change execution to the executor allows the executor more control over how changes are executed. This change, for example, a custom executor implementation that uses its own separate database connection to commit changes after the last change in a changeset.
… execution This commit adds the following - An AlternateConnectionExecutor that extends JdbcExecutor and uses its own, separate JDBC connection to a test database (H2) and overrides the execute method to commit after each Change. The connection used by this executor is to H2 and uses an alternate schema by default. - A test changelog whose change sets runwith the AlternateConnectionExecutor in order to create a table and then insert values into the table. The change intentionally fails in the final change of the last change set to help the test prove the preceding changes were committed. - An integration test that executes the change log and asserts that the table was created in the alternate schema with the expected number of rows and that the table does not exist in the default database.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
liquibase-integration-tests/src/test/java/liquibase/dbtest/h2/H2IntegrationTest.java
Fixed
Show resolved
Hide resolved
liquibase-integration-tests/src/test/java/liquibase/dbtest/h2/H2IntegrationTest.java
Fixed
Show fixed
Hide fixed
… execution This commit adds the following - An AlternateConnectionExecutor that extends JdbcExecutor and uses its own, separate JDBC connection to a test database (H2) and overrides the execute method to commit after each Change. The connection used by this executor is to H2 and uses an alternate schema by default. - A test changelog whose change sets runwith the AlternateConnectionExecutor in order to create a table and then insert values into the table. The change intentionally fails in the final change of the last change set to help the test prove the preceding changes were committed. - An integration test that executes the change log and asserts that the table was created in the alternate schema with the expected number of rows and that the table does not exist in the default database.
- Moving the checks into the changelog removes the need for the test to look up the executor - Executors defined in liquibase-standard are picked up in Ubuntu
|
@MalloD12 , I am running into different issues on this branch now when I build locally in ubuntu (fails on UpdateTestingRollbackCommandsIntegrationTest) and windows (fails in liquibase-cli). I don't see how what I changed affects those so I'm hoping it's just a local issue. |
|
Hi @MalloD12, looks like all the tests passed and it's failing on a Run functional test step. When I click the details link, I get a 404 page so I don't think I have permissions to see. Is there anything I need to look into related to that failure? |
|
Hi @dzeigler, Yeap, with your last change the test we were looking at started passing which is good. Now, yeah there is a functional test failing related to Thanks, |
|
|
||
| <changeSet id="3" author="your.name" runWith="h2alt"> | ||
| <preConditions onFail="HALT"> | ||
| <sqlCheck expectedResult="2">select count(*) from test_numbers where id in (1,2)</sqlCheck> |
There was a problem hiding this comment.
Wouldn't be enough only one of these sqlCheck without filtering by id and expecting 2 results?
There was a problem hiding this comment.
Yeah that'd be enough. I thought having the id there might make the test expectation more clear to someone reading, but you're right that checking the count is 2 would be a sufficient check.
| final String changeLogFile = "changelogs/common/runWith.executor.changelog.xml"; | ||
| try { | ||
| Liquibase liquibase = createLiquibase(changeLogFile); | ||
| liquibase.update(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
| // Confirm the number of expected rows were inserted into the table in the alt schema by running the changelog again to get to the precondition checks | ||
| try { | ||
| Liquibase liquibase = createLiquibase(changeLogFile); | ||
| liquibase.update(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
There was a problem hiding this comment.
Approved.
With latest changes made that we have been discussing last week and considering build and test checks have been successfully executed, I think this is ready to move forward.
Thanks @dzeigler for creating this PR and being very receptive with feedback provided here.
Daniel.
|
Hi @MalloD12 , |
Hi @dzeigler, To be included in the next release this still needs to be reviewed/tested by QA team, so then we can merge it. I think that will happen soon. We are expecting to have our next release end of this month or the beginning of February. Thanks, |
filipelautert
changed the title
Resolves #2696 by delegating Change execution to the executor's execute(Change, List) method
Impact
Description
AbstractJdbcDatabase delegates the execution of sqlstatements to executors. With this PR, AbstractJdbcDatabase delegates the execution of Changes to the executor too, which allows the executor more control over how Changes are executed. For example, a custom executor implementation that uses its own separate database connection can now have logic to commit after the last Change in a ChangeSet. Previously, the Executors had no access to the Change or ChangeSet and could not manage its transaction commits.
Things to be aware of
The change to AbstractExecutor brings its execute(Change change, List sqlVisitors) parity with execute(final SqlStatement[] statements, final List sqlVisitors).
Things to worry about
Additional Context