Bugfix/prod 399 triggered prediction error on public api production at 2022 07 29 t 16 07 15 529 z #12
Conversation
rmoneys
left a comment
rmoneys
left a comment
There was a problem hiding this comment.
Looks good. I hope you can fix my mistake in the tests.
| EventLogBuilder(event_log.as_uri(), temp_dir).build() | ||
| with self.assertRaises(ValueError, msg=msg): | ||
| event_log_paths = extractor.Extractor(event_log_path.as_uri(), temp_dir).extract() | ||
| eventlog.EventLogBuilder(event_log_paths, temp_dir).build() |
There was a problem hiding this comment.
Nice abstraction. I made an error when I coded the first version of this thinking that assertRaises will assert that the value of its "msg" argument matches that of the attribute of the exception. That's not the case: https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises
I should've done,
with self.assertRaises(ValueError) as cm:
event_log_paths = extractor.Extractor(event_log_path.as_uri(), temp_dir).extract()
eventlog.EventLogBuilder(event_log_paths, temp_dir).build()
assert str(cm.exception) == msg, "Exception message matches"
There was a problem hiding this comment.
Done! Had to fix some of the error messages to match, too.
btw, curious why you went went unittest here instead of using pytest's exception testing capabilities.
There was a problem hiding this comment.
I don't recall having made a deliberate choice between the 2, but I'm interested in differences that may exist between the 2 or examples with the pytest alternative. I do like that these related tests are grouped in a class, and that class offers assertRaises... felt right
There was a problem hiding this comment.
I think I just Googled "python test raise exception": https://stackoverflow.com/questions/129507/how-do-you-test-that-a-python-function-throws-an-exception
rmoneys
left a comment
rmoneys
left a comment
There was a problem hiding this comment.
Type annotation looks off, but that can be addressed now or later. Looks good!
spark_log_parser/eventlog.py
Outdated
| self.source_url, self.work_dir, self.s3_client, extract_thresholds | ||
| ) | ||
|
|
||
| def _validate_event_log_paths(self, event_log_paths: Path | str) -> Path: |
There was a problem hiding this comment.
I think you want list[Path] | list[str] and list[Path] here
There was a problem hiding this comment.
Best to fix in now :)
|
Kudos, SonarCloud Quality Gate passed!
|
gorskysd
deleted the
bugfix/PROD-399-triggered-prediction-error-on-public-api-production-at-2022-07-29-t-16-07-15-529-z
branch
A few things tackled as part of PROD-399, part of a sync_backend ticket to create a single entry point for the spark predictor.
.reset_index()after the sort.