Skip to content

Conversation

@batuhansk
Copy link
Contributor

As you know, top level collection objects are also valid JSON. But, Mocker wasn't support them because of the casting that made up in the MockingURLProtocol.swift

@batuhansk
Copy link
Contributor Author

refers to this issue #119

Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this makes sense! Can you update the tests as well to verify this is working as expected?

@batuhansk batuhansk requested a review from AvdLee August 8, 2022 13:58
Copy link
Contributor

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

Copy link
Contributor

@raphkoebraam raphkoebraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@AvdLee
Copy link
Contributor

AvdLee commented Aug 10, 2022

@batuhansk could you pull latest master into your PR so CI succeeds? Thanks!

@batuhansk
Copy link
Contributor Author

@batuhansk could you pull latest master into your PR so CI succeeds? Thanks!

sure, done!

@wetransferplatform
Copy link
Collaborator

Messages
📖

View more details on Bitrise

📖 MockerTests: Executed 26 tests (0 failed, 0 retried, 0 skipped) in 1.610 seconds

SwiftLint found issues

Severity File Reason
Error MockingURLProtocol.swift:41 Line should be 160 characters or less: currently 166 characters (line_length)
Error MockingURLProtocol.swift:44 Line should be 160 characters or less: currently 200 characters (line_length)
Warning MockingURLProtocol.swift:101 Line should be 140 characters or less: currently 147 characters (line_length)
Error MockingURLProtocol.swift:105 Line should be 160 characters or less: currently 221 characters (line_length)
Error Mock.swift:8 Line should be 160 characters or less: currently 177 characters (line_length)
Warning Mock.swift:49 Line should be 140 characters or less: currently 155 characters (line_length)
Error Mock.swift:81 Line should be 160 characters or less: currently 211 characters (line_length)
Error Mock.swift:84 Line should be 160 characters or less: currently 208 characters (line_length)
Error Mock.swift:93 Line should be 160 characters or less: currently 287 characters (line_length)
Warning Mock.swift:122 Line should be 140 characters or less: currently 142 characters (line_length)
Warning Mock.swift:129 Line should be 140 characters or less: currently 141 characters (line_length)
Error Mock.swift:136 Line should be 160 characters or less: currently 246 characters (line_length)
Error Mock.swift:137 Line should be 160 characters or less: currently 236 characters (line_length)
Warning Mock.swift:148 Line should be 140 characters or less: currently 150 characters (line_length)
Warning Mock.swift:149 Line should be 140 characters or less: currently 153 characters (line_length)
Warning Mock.swift:185 Line should be 140 characters or less: currently 159 characters (line_length)
Error Mock.swift:195 Line should be 160 characters or less: currently 168 characters (line_length)
Warning MockerTests.swift:163 Line should be 140 characters or less: currently 151 characters (line_length)
Warning MockerTests.swift:181 Line should be 140 characters or less: currently 149 characters (line_length)
Warning MockerTests.swift:239 Line should be 140 characters or less: currently 154 characters (line_length)
Warning MockerTests.swift:52 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:71 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:94 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:116 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:149 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:165 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:183 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:206 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:259 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:457 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:491 Chained function calls should be either on the same line, or one per line. (multiline_function_chains)
Warning MockerTests.swift:334 Lines should not have trailing whitespace. (trailing_whitespace)

Code Coverage Report

Name Coverage
Mocker.framework 87.93%

Generated by 🚫 Danger Swift against accd9f5

@AvdLee AvdLee merged commit e7165fb into WeTransfer:master Aug 10, 2022
@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 2.7.0 🚀

Generated by GitBuddy

Copy link
Contributor

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR made release v2.7.0 break the contract (APIs), thus who updated from 2.6.0, this change broke their envs. Was this expected? Shouldn't this have been a major version bump (e.g. v3.0.0)? 😬 @batuhansk @AvdLee

@AvdLee
Copy link
Contributor

AvdLee commented Oct 10, 2022

This PR made release v2.7.0 break the contract (APIs), thus who updated from 2.6.0, this change broke their envs. Was this expected? Shouldn't this have been a major version bump (e.g. v3.0.0)? 😬 @batuhansk @AvdLee

Ah, you're right! That went unnoticed, mostly since I think I thought Any would allow existing code to continue. We've didn't notice much issues on our side at least.

Did you run into many build errors?

@rogerluan
Copy link
Contributor

rogerluan commented Oct 10, 2022

Yes, our tests started failing upon upgrading from v2.6.0 to v2.7.0, so for now we locked on v2.6.0 (.exact("2.6.0")) until we (you and I?) figure out the versioning of Mocker... I think there are a few options:

  • a. Revert this PR, and re-release v2.7.0 without it (force push tags and so on - this requires rewriting git history). Then release this PR as part of v3.0.0 (since it includes a breaking change). Down side of this is that whoever has already updated their tests to the new interface will have to undo.

  • b. Revert this PR, and release v2.8.0. This doesn't require rewriting git history. There is no down side to this, except who decides to upgrade to the latest version again (v2.8.0) will have to revert their interface changes.

  • c. Keep everything as is and force everyone to update their code if they wanna use v2.7.0.

  • d. Create workarounds/adapt this PR to be backwards compatible in its interface. This requires re-thinking the approach of this PR, thus it's the highest-effort solution IMO. Might not even be possible without changing the interface. Once we figure out if this is possible, implement it and release as either v2.7.0 (force push), or a new v2.8.0.

Whatever way we decide, we have to inform users how to migrate from one to another I think (regardless if they're doing this in v2.7.0, v2.8.0 or v3.0.0). This can be done via a e.g. MIGRATION.md or just release notes.

WDYT @AvdLee ?

@AvdLee
Copy link
Contributor

AvdLee commented Oct 11, 2022

Could you show me the errors you got? Would be great if we can open a branch with that error reproduced. That way, I could play around with the API and make the code work for both backwards compatibility, as well for @batuhansk's case

@rogerluan
Copy link
Contributor

I don't have the build logs anymore and would take some time to reproduce the environment in which this occurs, but the error was here:

mock.onRequest = { _, body in
    guard let body = body else { return }
    XCTAssertEqual(body["answers"] as? [String], answers)
    parametersExpectation.fulfill()
}

body in v2.6.0 is a [String:Any] whereas in v2.7.0 it's Any. Perhaps if I blindly casted it to [String:Any] it would work, but I didn't check it back then and decided to go for the safest route first.

So the error was like "body of type Any doesn't have a subscript etc…" something along those lines :)

Does this help @AvdLee ? 🙇

@AvdLee
Copy link
Contributor

AvdLee commented Oct 17, 2022

@rogerluan makes sense! I've opened an issue here: #132

I can't promise any timelines here, but feel free to jump on it if you have the time!

@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 3.0.0 🚀

Generated by GitBuddy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants