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 CI check for unecessary template tests #6191

Merged

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Oct 12, 2023

When the regression tests output differs between PG versions we create a template test (aka *.sql.in) and add several different output files for each supported version.

Over time we support new PG version and deprecate older ones, for example nowadays we're working on PG16 support and removed PG12 so we need to check if we still need template files.

Disable-check: force-changelog-file

@fabriziomello fabriziomello added ci enhancement An enhancement to an existing feature for functionality labels Oct 12, 2023
@fabriziomello fabriziomello self-assigned this Oct 12, 2023
@fabriziomello fabriziomello force-pushed the ci_check_unecessary_template_tests branch 2 times, most recently from 6b98e4a to dfce181 Compare October 12, 2023 21:32
@fabriziomello fabriziomello marked this pull request as ready for review October 12, 2023 21:33
@github-actions github-actions bot requested review from akuzm and antekresic October 12, 2023 21:33
@github-actions
Copy link

@akuzm, @antekresic: please review this pull request.

Powered by pull-review

@fabriziomello fabriziomello force-pushed the ci_check_unecessary_template_tests branch 2 times, most recently from d06beb4 to bb51db3 Compare October 12, 2023 21:40
@fabriziomello fabriziomello enabled auto-merge (rebase) October 12, 2023 21:43
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #6191 (26d5c48) into main (5752c33) will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #6191      +/-   ##
==========================================
- Coverage   81.46%   81.40%   -0.07%     
==========================================
  Files         246      246              
  Lines       56954    56908      -46     
  Branches    12619    12600      -19     
==========================================
- Hits        46398    46326      -72     
+ Misses       8175     8171       -4     
- Partials     2381     2411      +30     

see 45 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fabriziomello fabriziomello force-pushed the ci_check_unecessary_template_tests branch from bb51db3 to eb89b73 Compare October 12, 2023 21:53
Copy link
Contributor

@jnidzwetzki jnidzwetzki left a comment

Choose a reason for hiding this comment

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

I like the idea of having this check. However, could you split the commit into (1) removing the telemetry test and (2) introducing this check?

In addition, the files for the telemetry_stats test seem to be different for the PostgreSQL versions.

$ diff --from-file ./tsl/test/expected/telemetry_stats-*
717c717
<     "num_reltuples": 357
---
>     "num_reltuples": 368
747c747
<     "num_reltuples": 340
---
>     "num_reltuples": 329
823c823
<         "uncompressed_row_count": 72,
---
>         "uncompressed_row_count": 56,
832c832
<     "num_reltuples": 285
---
>     "num_reltuples": 312
853c853
<         "uncompressed_row_count": 44,
---
>         "uncompressed_row_count": 60,
862c862
<     "num_reltuples": 296
---
>     "num_reltuples": 269

@fabriziomello fabriziomello force-pushed the ci_check_unecessary_template_tests branch from eb89b73 to c79a2f5 Compare October 13, 2023 13:17
@fabriziomello
Copy link
Contributor Author

diff --from-file ./tsl/test/expected/telemetry_stats-*

You're correct @jnidzwetzki ... the output are different, but in another PG16 branch that I'm working they will become equal due to the removal of some multi-node tests, so let's merge here just the new CI check.

When the regression tests output differs between PG versions we create a
template test file (*.sql.in) and add several different output files for
each supported version.

Over time we support new PG version and deprecate older ones, for
example nowadays we're working on PG16 support and removed PG12 so we
need to check if we still need template files.
@fabriziomello fabriziomello force-pushed the ci_check_unecessary_template_tests branch from c79a2f5 to 26d5c48 Compare October 13, 2023 14:08
@fabriziomello fabriziomello enabled auto-merge (rebase) October 13, 2023 14:26
@fabriziomello fabriziomello merged commit f12ed00 into timescale:main Oct 13, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci enhancement An enhancement to an existing feature for functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants