Skip to content

chore(tests): remove SkipIfPostgresEnabled#14749

Merged
janisz merged 3 commits intomasterfrom
remove_SkipIfPostgresEnabled
Apr 11, 2025
Merged

chore(tests): remove SkipIfPostgresEnabled#14749
janisz merged 3 commits intomasterfrom
remove_SkipIfPostgresEnabled

Conversation

@janisz
Copy link
Copy Markdown
Contributor

@janisz janisz commented Mar 25, 2025

Description

It's long time since we switch to postgres and it's time to cleanup.
SkipIfPostgresEnabled was introduced to skip tests that will not work on postgres and run them when the feature flag is disable. Over the time it evolved to skip test always (as feature flag was always on, and eventually removed). This PR removes this functions and fixes tests or remove them when the new postgres test was added.


  • CHANGELOG update is not needed
  • Documentation is not needed

Testing

  • inspected CI results

Automated testing

  • modified existing tests
  • contributed no automated tests

How I validated my change

CI

@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Mar 25, 2025

Images are ready for the commit at c0292c5.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-467-gc0292c5b6f.

Copy link
Copy Markdown
Contributor

@dashrews78 dashrews78 left a comment

Choose a reason for hiding this comment

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

Before removing datastore_impl_test.go please verify test parity with datastore_impl_postgres_test.go and either write a ticket or do follow on work to deal with any tests we should have in the postgres version. We found a while back that when the old cluster datastore tests were removed a lot of test coverage was removed and we recently added that back in. It would be nice to quantify that now vs accidentally find it later.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.00%. Comparing base (73d3730) to head (c0292c5).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14749      +/-   ##
==========================================
+ Coverage   48.95%   49.00%   +0.05%     
==========================================
  Files        2550     2550              
  Lines      187245   187245              
==========================================
+ Hits        91664    91765     +101     
+ Misses      88332    88228     -104     
- Partials     7249     7252       +3     
Flag Coverage Δ
go-unit-tests 49.00% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@janisz janisz requested a review from dashrews78 March 25, 2025 19:58
@janisz
Copy link
Copy Markdown
Contributor Author

janisz commented Mar 25, 2025

@dashrews78 I fixed tests (removed one that has comment to remove) and this lead me to find a ux bug in errors.
Thanks for motivation to do it!

@janisz
Copy link
Copy Markdown
Contributor Author

janisz commented Mar 26, 2025

/retest

@janisz janisz requested review from ajheflin and clickboo March 27, 2025 17:03
@janisz janisz force-pushed the remove_SkipIfPostgresEnabled branch from c690a58 to 9e32295 Compare March 27, 2025 17:06
@janisz janisz requested a review from dashrews78 March 28, 2025 14:40
@janisz
Copy link
Copy Markdown
Contributor Author

janisz commented Apr 1, 2025

@dashrews78 @clickboo PTAL

Copy link
Copy Markdown
Contributor

@dashrews78 dashrews78 left a comment

Choose a reason for hiding this comment

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

I think this is OK. I would prefer a sign off from @clickboo before merging though.

@janisz janisz force-pushed the remove_SkipIfPostgresEnabled branch from 20fdd97 to af43d84 Compare April 9, 2025 08:56
@janisz
Copy link
Copy Markdown
Contributor Author

janisz commented Apr 9, 2025

@clickboo I rebased on master. Please take a look

janisz added 3 commits April 11, 2025 10:41
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>

# Conflicts:
#	central/policy/datastore/datastore_impl_test.go
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>

# Conflicts:
#	central/policy/datastore/datastore_impl_test.go
Signed-off-by: Tomasz Janiszewski <tomek@redhat.com>
@janisz janisz force-pushed the remove_SkipIfPostgresEnabled branch from af43d84 to c0292c5 Compare April 11, 2025 08:41
@janisz janisz enabled auto-merge (squash) April 11, 2025 08:41
@janisz
Copy link
Copy Markdown
Contributor Author

janisz commented Apr 11, 2025

/retest

@janisz janisz merged commit b14b151 into master Apr 11, 2025
89 checks passed
@janisz janisz deleted the remove_SkipIfPostgresEnabled branch April 11, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants