Commit or rollback?

[yallie]

Hello Adrian,

just noticed that test_autonomous(p_statement) function performs COMMIT when the p_statement executed correctly. That of course means that running unit tests may leave their traces such as inserted/deleted records, etc.

I believe that the autonomous transactions of the unit tests should be rolled back in any case. What do you think?

[adrianandrei-ca]

Commit should be part of the test . Data cleanup should be also part of setup and tear down so database stays clean. It is a bad idea to use rollback as a way to keep the database clean IMHO.

…

On Dec 7, 2016 9:26 AM, "Alexey Yakovlev" ***@***.***> wrote: Hello Adrian, just noticed that test_autonomous(p_statement) function performs COMMIT when the p_statement executed correctly. That of course means that running unit tests may leave their traces such as inserted/deleted records, etc. I believe that the autonomous transactions of the unit tests should be rolled back in any case. What do you think? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR4ag8719J6fyRjO7Vh3Zx5Uuxb63ijks5rFsIagaJpZM4LGpn9> .

[yallie]

The rollback guarantees that the database will be left exact in the same state as before executing the test case. On the contrary, data cleanup performed by tear down function, can be flawed and we cannot guarantee anything about it.

What that means is that the broken test suite will probably damage the database which is something that's not supposed to happen. I believe that one should be able to run

select * from test_run_suite('business_logic_tests_by_junior_developer')

without expecting bad side effects from it.

[adrianandrei-ca]

I am against automatic side effects in testing. A test should consist in setup check and tear down. Tell the junior developer to write the tests this way. ☺

…

On Wed, Dec 7, 2016, 9:38 AM Alexey Yakovlev ***@***.***> wrote: The rollback guarantees that the database will be left exact in the same state as before executing the test case. On the contrary, data cleanup performed by tear down function, can be flawed and we cannot guarantee anything about it. What that means is that the broken test suite will probably damage the database which is something that's not supposed to happen. I believe that one should be able to run select * from test_run_suite('business_logic_tests_by_junior_developer') without expecting bad side effects from it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR4akCFc7L17kKY23mL5uW8VGc_VUwNks5rFsT0gaJpZM4LGpn9> .

[yallie]

Commit should be part of the test

Can you please explain why? I think that individual test cases should be transparent to the transactions. I mean that a test case shouldn't be aware of the results of other test cases in the same suite.

Tell the junior developer to write the tests this way. ☺

It's just like to say him to write good software and avoid bugs. No matter how we teach our developers they will write flawed code every now and then, unintentionally.

By the way, I've checked the behavior of other Postgres unit testing frameworks concerning transactions. Both PGUnit and pgTAP mentioned in Postgres documentation stick with rolling back the test transactions.

[yallie]

P.S. Same with Epic test framework (the website is now defunkt, but the code is mirrored here):

-- ALWAYS RAISE EXCEPTION at the end of test procs to rollback!
RAISE EXCEPTION '[OK]';

[adrianandrei-ca]

The point of a testing framework is to be as close as possible to the production environment. One can have a trigger on commit that will not be tested if there is a rollback. Space and performance is also impacted. The framework is one that relies heavily on autonomous transaction for this very reason. Also I was looking at the fact that I want the data in when a test fails so further debugging can be done. Another aspect was test chaining as I want the ability to split complex tests into smaller one and group them in suites without changing the actual code. That's why I wrote this one and nor use another one. Regards , Adrian

…

On Dec 7, 2016 10:01 AM, "Alexey Yakovlev" ***@***.***> wrote: Commit should be part of the test Can you please explain why? I think that individual test cases should be transparent to the transactions. I mean that a test case shouldn't be aware of the results of other test cases in the same suite. Tell the junior developer to write the tests this way. ☺ It's just like to say him to write good software and avoid bugs. No matter how we teach our developers they will write flawed code every now and then, unintentionally. By the way, I've checked the behavior of other Postgres unit testing frameworks concerning transactions. Both PGUnit <http://en.dklab.ru/lib/dklab_pgunit/> and pgTAP <http://pgtap.org/documentation.html> mentioned in Postgres documentation stick with rolling back the test transactions. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR4apaaZ6gppAk4GirhwA5KQoia4HWIks5rFspPgaJpZM4LGpn9> .

[yallie]

Thanks for the detailed reply Adrian! I see your point.

I chose your testing framework for another reason though. I like that test cases don't have to follow some strict protocol, such as returning specific data types, raising exceptions at the end, etc. I like this no-fuss minimalistic approach and hierarchical test suite grouping. Great stuff, very easy to use! 👍

One can have a trigger on commit that will not be tested if there is a rollback.

That's true, but the trigger logic can be extracted to a function and tested separately.

Space and performance is also impacted.

Either way, whether you commit or roll back the transaction, you have that transactional MVCC overhead.

Also I was looking at the fact that I want the data in when a test fails so further debugging can be done.

But then why do you roll back the autonomous transaction on exceptions?

Another aspect was test chaining as I want the ability to split complex tests into smaller one and group them in suites without changing the actual code.

Well, this particular problem can be addressed by running a transaction per test suite, not per individual test case.

Thanks, Alexey

[yallie]

Hello Adrian,

can we just make this behavior adjustable? I'd really like to continue using this framework, but this potential damage issue keeps troubling me. Also, I found that cleanup logic doubles the amount of the test code to be maintained. It can get quite complex for non-trivial tests, which isn't good, too (caught myself thinking about unit testing the cleanup logic).

Having added a little option we could keep best of two worlds by choosing either bulletproof-safe auto-rollback or not-so-safe but close to production auto-commit mode. Would you accept such a PR?

Regards, Alexey

[adrianandrei-ca]

Sure, as long it is not enabled by default please .

…

On Thu, Dec 8, 2016, 3:19 PM Alexey Yakovlev ***@***.***> wrote: Hello Adrian, can we just make this behavior adjustable? I'd really like to continue using this framework, but this potential damage issue keeps troubling me. Also, I found that cleanup logic doubles the amount of the test code to be maintained. It can get quite complex for non-trivial tests, which isn't good, too (caught myself thinking about unit testing the cleanup logic). Having added a little option we could keep best of two worlds by choosing either bulletproof-safe auto-rollback or not-so-safe but close to production auto-commit mode. Would you accept such a PR? Regards, Alexey — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQR4ajH63BTQjQBSIdj6NSlR69Wzs0O-ks5rGGY_gaJpZM4LGpn9> .

[yallie]

Cool, thanks.