Skip to content

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Jan 24, 2023

Issue

  • PR#53 silently broke the unit tests due to running CI on older ops version (1.5.4)

Solution

  • Fix issues resulting from the ops update

Context

  • One of the unit tests added in PR#53 doesn't seem to be possible anymore with the test harnesss relation broken functionality. It may be possible to reproduce it using mock events and calling the _on_relation_broken() directly
  • Also updating postgres_tls

Testing

Release Notes

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #57 (f4bf811) into main (28196bc) will increase coverage by 2.20%.
The diff coverage is 91.52%.

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   66.19%   68.39%   +2.20%     
==========================================
  Files           6        6              
  Lines         840      886      +46     
  Branches      121      133      +12     
==========================================
+ Hits          556      606      +50     
+ Misses        260      256       -4     
  Partials       24       24              
Impacted Files Coverage Δ
src/relations/db.py 72.72% <88.23%> (+2.97%) ⬆️
src/charm.py 61.20% <92.68%> (+3.64%) ⬆️
src/cluster.py 71.25% <100.00%> (+0.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dragomirp dragomirp marked this pull request as ready for review January 24, 2023 20:36
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

  1. we need to fix tests for sure
  2. deleting a valuable test part is strange... can we address (or at least report and workaround) runtime error?

@dragomirp
Copy link
Contributor Author

2. deleting a valuable test part is strange... can we address (or at least report and workaround) runtime error?

The test was added with PR#53 which broke the unit tests, so it is a new test. I think I should be able to rewrite the test by calling _on_relation_broken() directly instead of relying on the test harness, but that may take some time and slow down fixing the unit tests on main.

Copy link
Member

@marceloneppel marceloneppel left a comment

Choose a reason for hiding this comment

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

LGTM! I agree that we should keep the test, just fixing it. Thanks for fixing it @dragomirp! It seems that the ops code has some divergence with its own test code. We need to understand better and possibly open a bug.

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Yeah! A way better!

@dragomirp dragomirp merged commit 3f09edc into main Jan 25, 2023
@dragomirp dragomirp deleted the ops-154-unit-tests-fix branch January 25, 2023 16:51
BON4 pushed a commit to BON4/postgresql-operator that referenced this pull request Apr 23, 2024
* Fix unit tests

* Update postgresql_tls lib

* Readding test_on_relation_broken_extensions_keep_block

* Code review improvements
github-actions bot added a commit to canonical/test-runners-2-github-x64-postgresql-operator that referenced this pull request May 26, 2024
github-actions bot added a commit to canonical/test-runners-2-azure-arm64-postgresql-operator that referenced this pull request May 26, 2024
github-actions bot added a commit to canonical/test-runners-2-is-x64-postgresql-operator that referenced this pull request May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants