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

ci: postgres unittests configurations #28952

Merged
merged 12 commits into from
Jan 2, 2022
Merged

ci: postgres unittests configurations #28952

merged 12 commits into from
Jan 2, 2022

Conversation

c-p-b
Copy link
Contributor

@c-p-b c-p-b commented Dec 20, 2021

This PR simply enables running of postgres tests without doing anything else. It requires a couple of minor commits in Frappe repository: https://github.com/cpdeethree/frappe/tree/enable-postgres-tests

These commits are also included on the larger frappe and erpnext pr's with changes for postgres support for ERPNext:

#28359

Thus (minus the DEBUG branch pointer commit), all changes here are strict subsets of the above PR's

Depends on: frappe/frappe#15354

@ankush ankush self-assigned this Dec 20, 2021
@ankush ankush changed the title Enable postgres tests feat: make ERPNext installable on Postgres Dec 20, 2021
@ankush
Copy link
Member

ankush commented Dec 20, 2021

This is technically ready for review but can't be merged till frappe/frappe#15354 is merged, hence marking it draft for now.

Also I need to make this CI workflow conditional for postgres PRs.

@ankush ankush marked this pull request as draft December 20, 2021 05:41
@ankush ankush added postgres issue/pr related to postgres and removed dont-merge labels Jan 2, 2022
@ankush ankush added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Jan 2, 2022
@codecov
Copy link

codecov bot commented Jan 2, 2022

Codecov Report

Merging #28952 (d864972) into develop (ebc1039) will increase coverage by 5.99%.
The diff coverage is 100.00%.

❗ Current head d864972 differs from pull request most recent head 3871af2. Consider uploading reports for the commit 3871af2 to get more accurate results

@@             Coverage Diff             @@
##           develop   #28952      +/-   ##
===========================================
+ Coverage    49.69%   55.68%   +5.99%     
===========================================
  Files         1131     1131              
  Lines        67513    67516       +3     
===========================================
+ Hits         33548    37597    +4049     
+ Misses       33965    29919    -4046     
Impacted Files Coverage Δ
erpnext/regional/india/setup.py 95.36% <100.00%> (+0.07%) ⬆️
erpnext/utilities/product.py 14.70% <0.00%> (-35.30%) ⬇️
erpnext/shopping_cart/product_info.py 30.30% <0.00%> (-21.22%) ⬇️
...eport/item_variant_details/item_variant_details.py 84.33% <0.00%> (-3.62%) ⬇️
erpnext/education/doctype/student/student.py 73.68% <0.00%> (-3.16%) ⬇️
...ion/doctype/course_enrollment/course_enrollment.py 44.00% <0.00%> (-2.00%) ⬇️
...xt/stock/report/stock_analytics/stock_analytics.py 91.08% <0.00%> (-1.99%) ⬇️
...e/shopping_cart_settings/shopping_cart_settings.py 65.38% <0.00%> (-1.93%) ⬇️
...wise_balance_history/batch_wise_balance_history.py 91.37% <0.00%> (-1.73%) ⬇️
erpnext/shopping_cart/product_query.py 61.19% <0.00%> (-1.50%) ⬇️
... and 170 more

@ankush
Copy link
Member

ankush commented Jan 2, 2022

note: Reverted change to HSN code creation since it appears that something else is rolling back full transaction as apparent from CI run on this commit: d864972

https://github.com/frappe/erpnext/runs/4684613490?check_suite_focus=true

This is the root cause behind HSN codes not getting created properly in tests. I can fix this here but feels like this needs to be handled better by the framework. Improper error handling is causing rollback. Well, actually both changes are required, this kinda error handling in regional setup will cause weird state in DB.

for d in docs:
try:
doc = frappe.get_doc(d)
doc.flags.ignore_permissions = True
doc.insert()
except frappe.NameError:
frappe.clear_messages()
except frappe.DuplicateEntryError:
frappe.clear_messages()

cc: @gavindsouza @deepeshgarg007

for postgres DB we are just rolling back everything on SQL exception 😐

https://github.com/frappe/frappe/blob/96e1b7dcf9d39651aa72ea7af151aa15208e765b/frappe/database/database.py#L165-L166

@ankush ankush changed the title feat: make ERPNext installable on Postgres ci: postgres unittests configurations Jan 2, 2022
@ankush ankush marked this pull request as ready for review January 2, 2022 18:02
@ankush
Copy link
Member

ankush commented Jan 2, 2022

Merging just the CI change for now, rest will require a fix at framework level.

@ankush ankush merged commit d2074b1 into frappe:develop Jan 2, 2022
conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
Co-authored-by: Ankush Menat <ankush@frappe.io>
conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
Co-authored-by: Ankush Menat <ankush@frappe.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postgres issue/pr related to postgres squash Meant to tell reviewers that this PR should be squashed into a single commit while merging.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants