Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Das 1893 #8
Das 1893 #8
Changes from 1 commit
772f738
18c8de1
b28586e
b49fcf3
aa7b9e0
a9d7d7c
567abcf
258c800
597e5c5
85315c2
05b0b9b
506c118
6f22e2a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
would you consider renaming this to
expected_url_endpoint
and renaming headers_umm_var toexpected_headers_umm_var
?That would just help me see that these are going to be tested rather than used as set up input.
you can say no.
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.
So they will be used as the set up input. Unless I am misunderstanding you.
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.
Right, so the two variables I named, are what you expect the mock to be called with, they aren't input to the function called. And like I said, these are suggestions.
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 would move the setting of the status code from L775 to here like you do in the next subtest for code 200. Just for symmetry (and setting close to the testing location)
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.
Bit of a nit: Nice check for a failed status, but it's a bit weird seeing the JSON response for a successful request with an unsuccessful status.
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.
Side note - is it worth an extra assertion to know that
requests.put
was called with the expected JSON content and headers?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.
Yeah, I agree. I think that was the best way I thought for checking if the response failed but I can try tinker with some other ways.
I think since we already did the
mock_requests_put.assert_called_once_with()
test we don't need to have it here unless you really thinks it's helpful.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 have new commit that addresses this!
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.
It feels like something should be mocked here -
requests.put
again, maybe?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.
Yeah I can add that!
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.
Also - I feel like we need a sub test here for what happens when publishing the first UMM-Var record fails. (I think for now we agreed that we would continue on and try and publish the rest of the records, but I've been out a few days, so might be misremembering)
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.
Yes, that is what we agreed on. It continues with the ingest process. I will add another test like that. I wrote a comment below for @flamingbear regarding this.
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'm not sure how error handling works here.
What happens if one of the variables in the loop fails?
Then the return value would be something like
['FOO-EEDTEST', {'concept-id': 'FOO-EEDTEST'}]
?Who is in charge of checking for errors when you run publish_all_umm_var?
Should you raise exceptions rather than returning the full response on error?
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's a great question and something that @owenlittlejohns and other folks have been talking about. We don't want to raise an error because raising an error would stop the whole ingest process if a single variable in a collection has a hundred variables. We want to add it into the list and maybe have something that checks later for if they were any errors. But it is a good idea to add a test for checking if the list contains errors. I will add that.
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.
Agree we do not want to fail the "put" (ingest), but there is a good chance this will be updated/extended in future releases.
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.
Do we want to return the full response object, or maybe the error message? I can't remember the JSON format for a CMR ingest error, but maybe the JSON includes a field for the error message (rather than just using
response.text
).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 want to return the full response object. So to get the error it would be something like
response.json().['errors']
This is what
response.json()
looks like if there is an error: