Cassettes ordering can be changed in the multi-cassette mode

[selevit]

You use startmap to make the cassette list unique, but it leads to the situation when ordering of the cassettes can change, because map keys isn't ordered. I faced with this problem, when my tests failed in CI, but worked locally because of this.

https://github.com/kiwicom/pytest-recording/blob/e0ae83d20ea587fbad3e27e70fa011243dc7f2df/src/pytest_recording/_vcr.py#L45

BTW, what for do you make the cassette list unique? I would like to put one cassette multiple times in one testcase, so ensure that the same request is performed many times. Maybe just get rid of this?

P.S. Thanks a lot for the great lib, I really like it!

[Stranger6667]

Hi!

First of all, thank you for reporting this! I am glad you like the library :)

On the reported issue:

As far as I see, starmap exhausts the iterable, which, in this case, consists of loaded cassettes from the following paths:

• the default cassette path
• extra_paths (which goes in the order of items defined in the pytest marker) I.e., starmap applies the unpack function to all values in zip(*all_content), and I don't see where it might lead to changing the order.

The uniqueness is done on the unique function level, which iterates the input sequence without changing its order.

because map keys isn't ordered.

I am not sure what map keys do you mean in this situation. I don't see any mapping involved on this level. Could you elaborate, please?

Generally, it seems like I am missing something. Could you please share the failure you experienced? E.g., a CI log or something like this.

BTW, what for do you make the cassette list unique? I would like to put one cassette multiple times in one testcase, so ensure that the same request is performed many times. Maybe just get rid of this?

I think that there was no particular reason for that, and I can't think of any negative consequences / edge cases for such an approach. It could be the way to go! :)

[selevit]

I am not sure what map keys do you mean in this situation. I don't see any mapping involved on this level. Could you elaborate, please?

This is a part of CombinedPersister.load_cassette method:

 requests, responses = starmap(unpack, zip(*all_content))
 requests, responses = list(requests), list(responses)

Firstly, you convert the cassette list to map. And then you convert it back to the list. Here the ordering can be missed, because when you iterate over the map (when converting it back to the list), the ordering of the elements is not guaranteed.

Could you please share the failure you experienced? E.g., a CI log or something like this.

I can, if the code above wouldn't make the problem clear for you :)

It could be the way to go!

I can create a PR with this change then. WDYT?

[Stranger6667]

Firstly, you convert the cassette list to map.

It doesn't convert the cassette list to a map, it is a starmap object from the itertools module, which holds two references (besides the regular PyObject boilerplate) - one to the first argument, and the second to the iterator from the second argument. There is no map involved.

Also, the actual iterator yields pairs of two lists (requests / responses), as the actual loading happens outside of starmap, in the inner generator expression from the line 38.

I can create a PR with this change then. WDYT?

Sure.

I can, if the code above wouldn't make the problem clear for you :)

I'd appreciate it. It seems to me that the reported issue is not caused by the starmap usage.

[selevit]

It doesn't convert the cassette list to a map,

Yes, you're right my bad. I think the issue is not in the startmap func, but in unique one, which does actually return the map of the cassettes path.

[selevit]

Ah, hang on, sorry, unique uses the map only for checking if the value is seen.

[selevit]

Let me recheck.

[selevit]

I realised that VCR doesn't care about the cassette ordering. It just tries to find a suitable request and plays it.

https://github.com/kevin1024/vcrpy/blob/c79a06f639dd628536c9868044e78df1012985f9/vcr/cassette.py#L254

This method just checks, if request in self and that's it. It means that it doesn't make a lot of sense to remove unique from CombinedPersister.load_cassette, as we discussed before.

[selevit]

Sorry for taking your time on this, I'm gonna close the issue.

Thanks a lot for the fast response!

[Stranger6667]

@selevit No worries! I am happy to discuss any issues happening within the library's scope :)

Feel free to open another issue if there is anything we can improve