Extract code coverage jobs into separate CI workflow

[artemgavrilov]

Extract code coverage jobs into separate CI workflow and simplify it. Remove variables as we want to run this workflow only once as code coverage results don't depend on OS and build system. Also use only one job to avoid build artifact upload/download.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.10%. Comparing base (f3719a7) to head (463ebd2).

❌ Your project status has failed because the head coverage (76.10%) 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     #236      +/-   ##
=====================================================
+ Coverage              76.07%   76.10%   +0.03%     
=====================================================
  Files                     22       22              
  Lines                   2449     2448       -1     
  Branches                 385      384       -1     
=====================================================
  Hits                    1863     1863              
  Misses                   513      513              
+ Partials                  73       72       -1     

Components	Coverage Δ
access	72.41% <ø> (ø)
catalog	86.34% <ø> (+0.10%)	⬆️
common	92.50% <ø> (ø)
encryption	71.90% <ø> (ø)
keyring	69.87% <ø> (ø)
src	52.80% <ø> (ø)
smgr	96.96% <ø> (ø)
transam	∅ <ø> (∅)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dutow]

this doesn't have to be make-test-tde.sh, make-test is enough (and better) if we only want to run pg_tde tests

[artemgavrilov]

Sorry, I didn't get why it's better. Isn't make-test-tde.sh --continue --tde-only runs exactly only tde tests?

[dutow]

Yes, but it also sets up global tde settings for all testcases, and that's not ideal for pg_tde specific tests. We are only using that to make sure that the entire regression test suite can be executed with pg_tde.

[Naeem-Akhter]

@dutow, Please help me understand the difference between 'make test' and 'make-test-tde'?

As I understand, make-test is for verification of the community server/vanilla behavior with heap access method using check-world. Meanwhile, 'make-test-tde.sh' is used to verify the PSP server using the 'tde_heap' access method and pg_tde, and we use installcheck-world here.

[dutow]

Yes, that's the difference.

But --only-tde only runs the tests for pg_tde. In that case, we do not need make-test-tde, in fact, it is worse, because that means we are starting the pg_tde tests with tde setup even before the tests start executing, and that's not ideal there.

Adding this option to the tde test script was incorrect, that option should be part of the normal make-test script, or not even part of these scripts.

[Naeem-Akhter]

So, if I understood you correctly, we have an additional step ci_scripts/configure-tde-server.sh for make-test-tde, which calls ci_scripts/tde_setup_global.sql for setting up a server for the tde_heap access method. And you don't want to set it up before if we intend to execute the pg_tde test suite only; instead, just use ci_scripts/tde_setup.sql.

Then, why are we running the pg_tde test suite with check-world, where our primary goal is to verify the vanilla server behavior for just the 'heap' access method? Shouldn't pg_tde be disabled for 'heap' access method with check-world?

[Naeem-Akhter]

Maybe I'm missing something concerning the setup with heap/tde_heap vs check-world/install-check/install-check. I will catch up with you.

[artemgavrilov]

As I understood situation, we have two test tracks:

1. Run ordinary tests for world (test job in CI)
2. Run world tests on preconfigured PG with tde_heap as default access method (test_tde)

But for code coverage we want to run only TDE tests on PG with heap AM (default).

Jobs naming a little bit confusing, so I changed it a little bit

[jeltz]

Yes, skipping the pg_tde tests when running make-tde-test.sh would make sense and would remove the need to the alternate result files. But that is the subject for another PR.

[dutow]

While it makes more sense to keep it here, this is also more hidden, I'm not sure if it's a good idea or bad.

[artemgavrilov]

Then we can keep in main readme but mention that it's just tde coverage pg_tde coverage: <badge>

[artemgavrilov]

How about this?

[Naeem-Akhter]

Something like Code Coverage (pg_tde): <Badge> or Code Coverage for pg_tde: <Badge> will look better. But not a big deal. :)

[artemgavrilov]

Changed

[dutow]

I also wanted to create an improvement PR for this, as the current huge list of skipped test just adds to much noise to the CI report.

My idea was to use specific scripts instead (coverage-build, coverage-test), and add it properly to the matrix, and changing the if skip logic to exclude so it doesn't show up - but maybe this approach is better, as the duplication is only in the calls to the scripts. And we can improve it further in the future by moving some of it into a composite action or something.

[Naeem-Akhter]

I also wanted to create an improvement PR for this, as the current huge list of skipped test just adds to much noise to the CI report.

My idea was to use specific scripts instead (coverage-build, coverage-test), and add it properly to the matrix, and changing the if skip logic to exclude so it doesn't show up - but maybe this approach is better, as the duplication is only in the calls to the scripts. And we can improve it further in the future by moving some of it into a composite action or something.

Yeah, there is less noise, and I also wanted to have a separate job for Coverage, just like this; however, somehow I was under the impression that we could not go beyond matrix, psp-matrix, and reusable.

[dutow]

Shouldn't this also have a --continue flag? Otherwise a failing test will also result in a lower coverage, because the test stops early.

[artemgavrilov]

I assume you talking about make-test, not make-build. If some test failed coverage report would not be uploaded at all (and should not). As I understood --keep-going option in make is needed for situation where we have target A that depends on targets B and C, and if B failed with keep-going option make still will execute C. If that's how it work then it's redundant here as we run only pg-tde tests here, am I wrong?