Proof of concept: Ideas for cleaner TAP tests #249
Conversation
Some things tested in 001_basic.pl can easily be tested by pg_regress, so let's remove them from the TAP test. - Checking the extension version - Verify that tde_heap tables can't be created without principal key set - Select from table right after insert - Dropping a tde_heap table - Dropping the extension
Assume that everything that isn't the test subject works and don't
assert that it does. These things can easily be tested by pg_regress.
Also prefer q{} over '' for SQL strings as that will keep escaping to a
minimum and make it easier to read.
Instead of our home brewed assertion system we use regular Test::More assertions as these will document what is being tested and give a comprehensive error message if a test fail. Also use the "standard" test scrip pre-amble from src/test/perl/README
Just because these are tests we don't have to make the SQL hard to read.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project status has failed because the head coverage (77.13%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## TDE_REL_17_STABLE #249 +/- ##
=====================================================
+ Coverage 75.45% 77.13% +1.68%
=====================================================
Files 22 22
Lines 2465 2489 +24
Branches 388 394 +6
=====================================================
+ Hits 1860 1920 +60
+ Misses 532 490 -42
- Partials 73 79 +6
🚀 New features to boost your workflow:
|
This is by no means the final wrapper we should use, but should hide a peeve for Andreas so he can see the rest of this branch :D .
| SELECT * FROM test_enc ORDER BY id ASC | ||
| } | ||
| ), | ||
| "1|foobar\n2|barfoo", |
There was a problem hiding this comment.
Like I wrote directly to Anders I worry that this kind of comparison will result in a pretty hard to read output and that we need to split the output but in general I think this is moving us in the right direction. Need to test this locally a bit tomorrow and see if we are fine with it as-is or if we need to write the better helper already.
There was a problem hiding this comment.
I prefer the "output comparison" style tests, but maybe that's because I'm used to how mysql works - there the test framework is a combination of the SQL tests and the tap tests, basically the SQL tests have way more features.
Here the only reason why we have to use tap at all is because the sql tests are missing some features, mainly proper server restarts.
There was a problem hiding this comment.
I think that style of test just leads to higher maintenance cost without giving any value. I would probably prefer if we mainly did TAP tests, so I understand I'm the odd man out.
I just don't see the appeal of having to update the expectations for every single test if you change the output of a function, even if that function is only run as part of setting up scaffolding for every test except the one that actually tests that function.
I think there only need to be one test that asserts that CREATE EXSTENSION pg_tde works for example, and the rest can just assume that it works and focus on asserting whatever their test subject is.
There was a problem hiding this comment.
Here are two examples of the high maintenance cost of the regression tests I've fixed recently:
Test didn't actually test anything, it only setup scaffolding: ce63840
Test asserted the complete opposite of what it claimed to test: 1390dd0
These issues are more or less unavoidable in regression tests, but with proper TAP tests we can at least avoid them there.
There was a problem hiding this comment.
Yes, that is annoying, mysql mtr has special commands for that, disable_query_log, enable_query_log and disable_result_log, enable_result_log - statements written between those, or their results, won't get written to the output file.
| SELECT * FROM test_enc ORDER BY id ASC | ||
| } | ||
| ), | ||
| "1|foobar\n2|barfoo", |
There was a problem hiding this comment.
I prefer the "output comparison" style tests, but maybe that's because I'm used to how mysql works - there the test framework is a combination of the SQL tests and the tap tests, basically the SQL tests have way more features.
Here the only reason why we have to use tap at all is because the sql tests are missing some features, mainly proper server restarts.
I applied some ideas on how to clean up our TAP tests to 001_basic.pl for your consideration. Any feedback is appreciated.
I tried to split it up into commits that explains why I did what I did.
Also, the rules used for pgperltidy are disgusting. This code could have been so much nicer with sane perltidy rules, who was the dumbass who suggested we should run that 😆 .