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 tests with success messages for locale creation, editing, and deletion #763

Merged
merged 3 commits into from
Feb 4, 2024

Conversation

ACK1D
Copy link
Contributor

@ACK1D ACK1D commented Jan 7, 2024

Added tests for #762 (success messages for create, edit, and delete locale). Some logic has been refactored into BaseLocaleTestCase, and correcting some tests for use it. Please review code and let me know if need some work.

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (bb04c76) 92.64% compared to head (9690800) 92.73%.
Report is 1 commits behind head on main.

Files Patch % Lines
wagtail_localize/locales/tests.py 99.15% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   92.64%   92.73%   +0.09%     
==========================================
  Files          47       47              
  Lines        4065     4130      +65     
  Branches      602      603       +1     
==========================================
+ Hits         3766     3830      +64     
  Misses        176      176              
- Partials      123      124       +1     

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

@ACK1D
Copy link
Contributor Author

ACK1D commented Jan 7, 2024

Hi @zerolab
I've made some changes to code, and it would be helpful to update the Codecov report. Could you please trigger a new run? Thank you!

@zerolab
Copy link
Collaborator

zerolab commented Jan 8, 2024

@ACK1D thank you very much for this. I am not sure how to make Codecov update its comment, but https://app.codecov.io/gh/wagtail/wagtail-localize/pull/762 shows a +0.11% increase

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

@ACK1D kudos for expanding the test locales test coverage.
I left a few notes around the consistency of self.client.get/post vs self.get/post which would be nice to tidy up and be consistent about.

self.assertEqual(
response.context["form"].fields["language_code"].choices, [("fr", "French")]
)

def test_create(self):
response = self.post(
Copy link
Collaborator

Choose a reason for hiding this comment

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

TestLocaleCreateView still has a post method that posts to reverse("wagtaillocales:add"). I'd say keep using it, especially that we're using that in at least one other place

@@ -190,10 +277,14 @@ def post(self, post_data=None, locale=None):
)

def test_simple(self):
response = self.get()
# Test rendering the edit view with simple data
response = self.client.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

we now seem to have a mix of self.client.get like here, and self.get below (like in test_invalid_language). It would be nice to be consistent

@ACK1D ACK1D requested a review from zerolab February 2, 2024 18:22
Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Done is better than perfect. Thanks @ACK1D for the tweaks

@zerolab zerolab merged commit 7d9fc03 into wagtail:main Feb 4, 2024
13 checks passed
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