venmo / DVR

Network testing for Swift
http://blog.soff.es/network-testing-in-swift-with-dvr
MIT License
651 stars 86 forks source link

Version 1.3.0 is breaking: cassettes cannot be found #85

Closed loudmouth closed 5 years ago

loudmouth commented 5 years ago

Unfortunately I have not been able to pinpoint the issue yet (I have taken a look at the changes from 1.2.0 to 1.3.0) but I can share that I have a branch where all my tests correctly find the relevant cassettes on version 1.2.0 (and Xcode 9), but with 1.3.0 (and Xcode 10) the tests crash as the cassettes can't be found.

Something about the diff between 1.2.0 and 1.3.0 is causing this change. Any ideas what may be the issue?

If we can't pinpoint the problem, then maybe we should deprecate 1.3.0 and release it as 2.0.0 instead since the changes are seemingly breaking behavior.

dmiluski commented 5 years ago

Hi @loudmouth , breaking cassette? Is this a cassette that's part of DVR? Unit Test? Or a personal one? (related to Xcode 10, and new hasher protocol?)

I recently discovered an issue we are correcting locally, in which a DVR cassette could not be found b/c of a multi-part form encoded header uses the new hasher protocol and intermittently fails b/c of an indeterminate order.

Is this related?

loudmouth commented 5 years ago

Hey @dmiluski it's a cassette that's included in the unit test target of my own project that cannot be found.

I think it's possible that the issue you described is the same one. Do you have any tips on how I could verify if it's the same?

loudmouth commented 5 years ago

Some more info on this:

I'm now running Xcode 10 GM. I need to finish up the current feature work I'm doing so I decided to simply re-record the recordings. After running the tests and while recording and watching the tests succeed, I grabbed the file DVR recorded and included it in my project, ensuring that the file was added to the relevant test target package. Upon running the same tests again, the cassette cannot be found.

I will get a branch of my project showing the issue and push it soon.

dmiluski commented 5 years ago

@loudmouth

https://swiftunboxed.com/protocols/hashable/ Starting in Swift 4.2, hash values are seeded with a random value at launch. During the lifetime of the program, the hash values will be stable.

Any chance you already migrated to swift 4.2?

dmiluski commented 5 years ago

In our case, we had a specific test which was failing, which came from our multi-part requests. Quite confused by the issue, I tore down the failing test to compare raw data to data. Upon investigating, I noticed that the great majority was the same except for maybe 30 characters which were just ordered differently. https://github.com/venmo/DVR/blob/d41094d26e4d97e20a79e8260ecff84e4b36ff66/Sources/DVR/Cassette.swift#L57

The content that was ordered differently turned out to be the below BodyPart, which encodes these headers into the data which DVR then compares for reference. B/c of the new random seeded behavior, this order isn't preserved leading to intermittent failures. In this case, it turns out Apple already offered another model we could use to maintain order. https://developer.apple.com/documentation/swift/dictionary

Which lead to a very mall code change to address.

public struct BodyPart: FormDataType {
-    private let headers: [String: String]                                // Order not preserved between tests
+    private let headers: DictionaryLiteral<String, String> // Order preserved

So going back to the root of the issue, I'm assuming this may be related to some latest usage of Swift 4.2/Xcode 10, and not specifically a difference between 1.2 vs 1.3?

But if you have a failing test you can share, happy to take a further peak.

dmiluski commented 5 years ago

Scratch that. Still seeing the failure. Will follow up once I find out what I missed.

dmiluski commented 5 years ago

Coming back to this, I had to do a two part change in our SDK code which uses DVR to validate.

One part was using the DictionaryLiteral above to preserve order. The other change was to update how our arguments were passed in:

// These parameters needed to be in the same order as the pre-recorded cassettes, otherwise, I would need to re-record to gain this input ordering.
let parameters: DictionaryLiteral<String, String> = [
              "keyA": "<value>,
             "keyB": <value>,
              "keyC": <value>,
          ]

This was a bummer but it our case wasn't. major blocker since the majority of our request/response tests are for serialization/deserialization or general client sdk handling of which deserialization didn't require an order.

If your use cases are more complex this this (🤞), we may need to reconsider how we determine equivalent responses.

But if not, we may be able to close out this issue. with Swift 4.1.x+ approach of DictionaryLiterals?

@loudmouth , is this approach useful? Or is your test failures much more problematic?

loudmouth commented 5 years ago

Hey @dmiluski

Sorry for the delay. Here is a link to a set of test cases within one XCTestCase subclass that are failing. One thing to note about my project setup, is that instead of configuring each test with it's own DVR session, my HTTP client is static on the XCTestCase subclass. Before all tests start, i begin recording/replaying and after all tests in the class finish, i stop recording/replaying. Therefore, my cassette has many recordings, not just one. I do this as I have well over a hundred network acceptance tests and recording/crashing the test runner each time i need to update is not scalable.

In case it's helpful, here is the helper code where I configure DVR.

Regarding dictionaries, there is no way to guarantee the order of keys in a dictionary so I think maybe an array of tuples (String, AnyValueType) would be better as order is deterministic with an array.

Edit: maybe i glossed over the DictionaryLiteral bit. Do they preserver order?

dmiluski commented 5 years ago

@loudmouth , yes DictionaryLiteral preserves order. It's name does not convey that, but the documentation does. https://developer.apple.com/documentation/swift/dictionaryliteral

It's hardly a dictionary at that. Which is why in Swift 5.0, it will be updated: https://github.com/apple/swift-evolution/blob/master/proposals/0214-DictionaryLiteral.md

That being said, seeing if that helps? If it doesn't I don't have a ton of time to do support work for DVR to migrate to a Tuple interface for ordered comparisons. So I'm hoping this approach suffices for a short term approach? Otherwise, we may need to discuss further support options/approaches.

loudmouth commented 5 years ago

@dmiluski I am just getting back to this today. While I understand the theory that you used a dictionary literal to preserver the order of your headers, I'm not sure I understand how the interaction with DVR works.

In my code, I simply do the following:

let dvrSession = DVR.Session(cassetteName: cassetteName, backingSession: client.urlSession)
client.urlSession = dvrSession

And then in the static methods for XCTestCase setUp and tearDown which are called at before all tests in a class and after all tests in a class (as opposed to the instance methods which are called before each test) I begin recording and stop recording respectively.

Is there code I should be modifying in my code base to ensure my recordings are found?

dmiluski commented 5 years ago

Hi @loudmouth , is there any public API I could play with that causes the issue you're running into? Fortunately/Unfortunately our recordings have been done for quite a while and exist in our local unit test target. So I don't have a reproducible issue like yours just yet.

I was debating if there was something that may require a sort to resolve? But wasn't sure where to make that determination?

dmiluski commented 5 years ago

We also integrate DVR as a submodule which can make it a bit easier to debug. https://github.com/venmo/DVR/blob/d41094d26e4d97e20a79e8260ecff84e4b36ff66/Sources/DVR/Cassette.swift#L57

I was seeing if you are making it to this method and failing on an expected known success comparison? (When testing a single cassette with known request?)

loudmouth commented 5 years ago

Ok, the issue was that I was using a dictionary internally to represent my URL parameters before serializing a URL string—with the change to the way hash values are seeded in Swift 4.2, the url that was serialized varied on different runs and therefore DVR was unable to consistently match URLs stored in Cassettes with those that were being generated at runtime.

The solution was simple, just sort the url key-value pairs (alphabetically by keys). Everything is working now on my end. I can confirm that the underlying issue was not with DVR so I am marking this closed.