[recorder] Improved getUniqueName #6345
Conversation
|
nit: In practice the array is likely very small so this doesn’t matter. But usually for checking membership a set type of data structure is used? |
I changed it to be |
sdk/test-utils/recorder/src/utils.ts
Outdated
| Math.floor(Math.random() * 10000).toString(), | ||
| 5, | ||
| "00000" | ||
| )}`; | ||
| if (namesAlreadyTaken[name]) { | ||
| throw new Error(`Test name: ${name} is duplicated.`); |
There was a problem hiding this comment.
The error might be a bit misleading.
Also, it's better to have this namesAlreadyTaken at the getUniqueName function which is exposed from the Recorder object.
And throw an error saying..
Label <label> is already taken for the corresponding prefix, please provide a different prefix <prefix> OR give the same prefix with a new label.
There was a problem hiding this comment.
Oh! I see. I pushed something that seems to tackle this issue better. Let me know if this works.
There was a problem hiding this comment.
No, the new code misses the case where two different prefixes are trying to get the label. According to the recorder logic, it is not allowed(meaning playback would fail).
The older namesAlreadyTaken was fine, but put that here and use it for the label(labels cannot be repeated) -
Is that clear?
There was a problem hiding this comment.
Pushed a commit to solve the issue without the need for a new array/set.
|
Updated getUniqueName checks and throws an error if a label is already used.
|
HarshaNalluru
added
Client
| getUniqueName: function(prefix: string, label?: string): string { | ||
| let name: string; | ||
| if (!label) { | ||
| label = prefix; | ||
| } | ||
| if (isRecordMode()) { | ||
| name = getUniqueName(prefix); | ||
| recorder.uniqueTestInfo["uniqueName"][label] = name; | ||
| if (recorder.uniqueTestInfo["uniqueName"][label]) { |
There was a problem hiding this comment.
If we wanted to avoid any potential problem with weird labels e.g. toString() or __proto__, we could use a Set here instead of an object.
There was a problem hiding this comment.
Thanks, leaving it as it is for now since it is of low priority and will take up a lot of efforts to revamp the recordings. Might revisit later.
There was a problem hiding this comment.
labels like "toString()". Currently I believe we only use this for getting unique names for Azure resources and label is provided by users if they want to use a pre-existing prefix so we probably don't need to care for edge cases.
There was a problem hiding this comment.
Yup, discussed offline with @xirzec !
This is a simplistic solution. The idea is that, when the tests run, they will store these names in an in-memory array and throw if the name happens to be repeated. It's cheap enough.
I see a random number generator in the name, I wonder if we should remove that.
Fixes #5698