Skip to content

OSError exception caught #621

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

OSError exception caught #621

wants to merge 4 commits into from

Conversation

sumit-badsara
Copy link
Contributor

Description

Currently, In case of passing an invalid date to function, the expected exception was ValueError, but Windows raises OSError which is not being caught right now. So, the tests fail for a windows machine.
I have added OSError exception and added a corresponding message.
In the test file, I have not checked for a specific message as there are multiple responses possible, but I have left the status code assertion in that specific request, which will ensure the test will be still working and doing it's job.

Fixes #618

Type of Change:

  • Code
  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

python -m unittest discover tests

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged
  • Update Postman API at /docs folder
  • Update Swagger documentation and the exported file at /docs folder
  • Update requirements.txt

Code/Quality Assurance Only

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been published in downstream modules

SanketDG
SanketDG suggested changes Jun 3, 2020
View reviewed changes
tests/mentorship_relation/test_dao_creation.py Outdated
@@ -241,7 +241,7 @@ def test_dao_create_mentorship_relation_with_good_args_but_invalid_timestamp(sel

result = dao.create_mentorship_relation(self.first_user.id, data)

self.assertEqual(messages.INVALID_END_DATE, result[0])
# self.assertEqual(messages.INVALID_END_DATE, result[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to assert for the message you just wrote above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I was planning to do something else, but now plans changed! 😄

app/messages.py Outdated
timestamp is invalid."""
}
OSERROR_OR_INVALID_END_DATE = {
"message": """Unexpected Error Occured! If using windows,check
Copy link
Contributor

Choose a reason for hiding this comment

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

The date is not invalid. It's just way too large.

Read through https://docs.python.org/3/library/datetime.html#datetime.datetime.fromtimestamp and please write a better message. Let me know if you need any help.

@sumit-badsara
Copy link
Contributor Author

@SanketDG Actually for Windows, OSError is being thrown for smaller values, where python throws ValueError.
For the updated value, Both Linux and Windows are throwing OSError, Hence this test works in linux and windows both.

I guess additionally a new issue needs to be created regarding creating more test case for catching ValueError (OSError in case of Windows).
A Single test case won't be enough for this.

@SanketDG
Copy link
Contributor

SanketDG commented Jun 4, 2020

@BadduCoder I am pretty sure ValueError is also raised when the value is large.

Please don't try to remove that.

You have to account for both OSError and ValueError in the test that is failing.

You have to create a separate unit test for this case where Windows fails on a much smaller value.

@vj-codes vj-codes added the Status: Changes Requested Changes are required to be done by the PR author. label Nov 4, 2020
@vj-codes
Copy link
Member

vj-codes commented Apr 2, 2021

@BadduCoder can you please address the above changes

@vj-codes
Copy link
Member

Closing due to inactivity
Thank you for your contribution @BadduCoder

@vj-codes vj-codes closed this Apr 11, 2021
@isabelcosta isabelcosta mentioned this pull request Jul 10, 2021
Open
epicadk
epicadk reviewed Jul 10, 2021
View reviewed changes
app/api/dao/mentorship_relation.py
@@ -51,7 +51,7 @@ def create_mentorship_relation(self, user_id: int, data: Dict[str, str]):

try:
end_date_datetime = datetime.fromtimestamp(end_date_timestamp)
except ValueError:
except OSError:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an OS error though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epicadk epicadk epicadk left review comments

@SanketDG SanketDG SanketDG requested changes

Assignees
No one assigned
Labels
Status: Changes Requested Changes are required to be done by the PR author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OSError in Windows not being caught
4 participants
@sumit-badsara @SanketDG @vj-codes @epicadk