-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[recorder] binary response encoded in 'hex' do not work well #10009
Comments
Is this issue in Node or browser, or both? For Node, according to nock doc, the binary response body should be decoded using |
The test is under node folder, executes only in node.
We may need to tweak the recording while being saved. @ljian3377 When I execute the test mentioned in the issue with my storage account, I'm running into an error with 404 status code for the |
The API should support both browser and Node in the end. We are still working on it. I couldn't find a way to get the encoding of the ReadableStream so not able to workaround accordingly. @HarshaNalluru Yes, the subscription needs to register the quick query feature and probably it now only works in central Canada and France in production. I will ping you with the account info I am using. |
Here's a fix in a draft that works for these "query" tests - draft #10048 Adding a new function(private) that helps to update the fixture before the recording is saved, by wrapping the hex-value string with |
That's great. Wonder if the test recording in browser has the same problem. |
With #10048, still have a recorder issue when I fetch the same avro blob three times with different offsets. One of the recorded reply doesn't wrap the response with a @HarshaNalluru Could you take another look? |
@ljian3377 That's interesting. I believe this is what you are referring to. Line 382 in 317b9d8
I think I know what went wrong... |
Got another wierd recorder issue. The tracing case wasn't recorded correctly. azure-sdk-for-js/sdk/storage/storage-blob-changefeed/test/blobchangefeedclient.spec.ts Line 156 in 8eb7159
Serveral requests are not correctly recorded. I can't figure out why. The case is not much different except that it add a tracing option. |
We do have tracing tests that work fine in playback in storage-blob.
Not sure why an issue would come up now. |
@ljian3377 I have a repro. it.only("tracing - customize", async () => {
const pageIter = changeFeedClient.listChanges();
await pageIter.next();
}); As you have mentioned in the previous comment, looks like it is the last request that was not saved in the recording and that resulted in playback failure since the request wasn't saved. I still need to understand the root cause. |
Weird. Then why were other cases properly recorded? |
@HarshaNalluru it.only("tracing - customize", async () => {
const pageIter = changeFeedClient.listChanges({
// tracingOptions: { spanOptions: { parent: rootSpan.context() } }
});
await pageIter.byPage().next();
}); then it recorded properly. By calling |
My best guess is that the "Content-Length" is high(significantly high) for that specific request which might somehow be causing it not to record. It's not because it was the last request. I've tested by adding a new HTTP request at the end of the test which does get recorded but not the request that you pointed out. Needs a deeper investigation to even confirm my theory, I recommend skipping the test for now. |
@ljian3377 Oops, missed the comment.
I don't think so. The playback still failed. (You may confirm.) In the non-byPage case, 6/7 requests were recorded, the last one was not recorded. |
I changed my implementation to download ranges of blob instead of the whole blob. Now tracing case records correctly (with your fix in #10892). But another case is now giving error. azure-sdk-for-js/sdk/storage/storage-blob-changefeed/test/blobchangefeedclient.spec.ts Line 34 in f03960c
|
…le decoding hex values into binary (#10892) * allow 206 status code * changelog * changelog * Fix typo * allow 200-299 status codes * Update test * Changelog
#10892 has been merged. I'll get to the missing request issue soon! |
Hi @ljian3377, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support. |
When the response body is binary data, our recoder will store the data in hex.
When consuming the data in playback mode, we can't infer the encoding:
response.readableStreamBody?.readableEncoding = null
.If I set the encoding manually with
response.readableStreamBody?.setEncoding("hex");
The data get decoded correctly. Yet the length of data is not right. As in live mode, we are expecting binary data. Withstream.read(size)
, we mean to the size of bytes(8 bits). But for 'hex' data, each byte/char only contain 4 meaningful bits.Ran into this with the tests for quick query and change feed.
azure-sdk-for-js/sdk/storage/storage-blob/test/node/blobclient.spec.ts
Line 351 in 66799d5
The text was updated successfully, but these errors were encountered: