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

test: test failure due to rounding #28945

Merged
merged 1 commit into from
Dec 18, 2021
Merged

test: test failure due to rounding #28945

merged 1 commit into from
Dec 18, 2021

Conversation

rtdany10
Copy link
Contributor

@rtdany10 rtdany10 commented Dec 18, 2021

FAIL  test_average_ratings_on_feedback_submission_and_cancellation
(erpnext.hr.doctype.interview_feedback.test_interview_feedback.TestInterviewFeedback)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/frappe-bench/apps/erpnext/erpnext/hr/doctype/interview_feedback/
        test_interview_feedback.py", line 62, in test_average_ratings_on_feedback_submission_and_cancellation
    self.assertEqual(avg_on_interview_detail, round(feedback_1.average_rating))
AssertionError: 3.5 != 4

Rounding produces different result in case of averages with decimal and hence the assertEqual fails.

@codecov

This comment has been minimized.

@rtdany10
Copy link
Contributor Author

rtdany10 commented Dec 18, 2021

Issue arises only if the average has decimal. Since random function generates ratings, the average might or might not have decimal. I assume, so far, the test passed cause the average didn't have a decimal and hence the round function really didn't have any effect? 😕

https://github.com/frappe/erpnext/blob/develop/erpnext/hr/doctype/interview_feedback/test_interview_feedback.py#L95-L101

P.S Tests all passing 😄

@rtdany10
Copy link
Contributor Author

@Mergifyio backport version-13-hotfix

@mergify
Copy link
Contributor

mergify bot commented Dec 18, 2021

backport version-13-hotfix

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]

@ankush
Copy link
Member

ankush commented Dec 18, 2021

this is failing because of frappe/frappe#15203

Change in code is required, instead of fixing test 😄

@rtdany10
Copy link
Contributor Author

rtdany10 commented Dec 18, 2021

Change in code is required, instead of fixing test

Skeptical of that since the code assigns the value as it should, without rounding (?) 😕

entry.average_rating = self.average_rating

@rtdany10
Copy link
Contributor Author

rtdany10 commented Dec 18, 2021

My bad. Ratings are divided by number of stars and stored in db now.
So, yes, the code requires change. Shall I try fix it on this PR itself?

@ankush

@ankush
Copy link
Member

ankush commented Dec 18, 2021

Let's leave this for now, we will soon start allowing half-star ratings so then rounding won't make much sense.

ref: frappe/frappe#15203 (comment)

@ankush ankush merged commit 0ca467a into frappe:develop Dec 18, 2021
@ankush ankush changed the title fix: test failure due to rounding test: test failure due to rounding Dec 18, 2021
mergify bot pushed a commit that referenced this pull request Dec 18, 2021
@mergify
Copy link
Contributor

mergify bot commented Dec 18, 2021

backport version-13-hotfix

✅ Backports have been created

conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
conncampbell pushed a commit to conncampbell/erpnext that referenced this pull request Jan 9, 2022
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.

None yet

2 participants