-
Notifications
You must be signed in to change notification settings - Fork 25
Fix export an empty maintenance would crash #73
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
Conversation
| assert report.last_updated is None | ||
|
|
||
| # export empty report shouldn't have problem | ||
| report._export_dict() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may validate the last_updated as current date as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't remember how the timestamp was tested..., in this case, what I can think of is mock the datetime.datetime.utcnow() with a fake timestamp and check it, but doesn't make much sense to me. Do you think this is good or just a waste of lines? any other test methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can check the output timestamp, see if it's a timestamp with right format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the timestamp to be the current time/mocked time in case of an empty report would have been better. But looking at the bigger picture, adding the timestamp is to ensure the report doesn't crash. So checking if timestamp exist should be fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current_date = datetime.datetime.now()
with patch.object(datetime, "datetime", Mock(wraps=datetime.datetime)) as mocked_datetime:
mocked_datetime.now.return_value = current_date
report = MaintenanceReport()._export_dict()
assert report.get('last_updated') == datetime.datetime.strftime(current_date, "%Y-%m-%dT%H:%M:%SZ")
This is what I did though to check current time
|
|
||
|
|
||
| def write_timestamp(value): | ||
| # value could be None if the report is empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better to keep this method generic and move setting the value to current date in _export_dict()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I did it here since the MaintenanceEntry object could also have the same problem, which has a timestamp, defaulted to None as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fair. Since this is a generic method and not tied to maintenance_report only, please modify this comment to something like "defaults to current time if value is None"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense.
Now if the report is empty and last_updated field is None, then while exporting report, current datetime will be used to aviod crashing.
9bdb066 to
c66b53f
Compare
Now if the report is empty and last_updated field is None, then while
exporting report, current datetime will be used to aviod crashing.