Added tests for Single+Response and refactored Shared Test Helpers #1229

freak4pc · 2017-08-16T19:42:28Z

This PR adds tests for Single+Response (for RxMoya), fixing issue #1227.

This PR also:

Wraps Test Helpers converting Response to Single/Observable under RxTestHelpers.swift.
Moves Test Helpers related to generating Test Images to TestHelpers.swift.
Reorganize TestHelpers.swift with some MARKs and sectioning of helper methods / classes.

MoyaBot · 2017-08-16T19:53:27Z

SwiftLint found issues

Warnings

File	Line	Reason
TestHelpers.swift	3	Limit vertical whitespace to a single empty line. Currently 2.
Single+MoyaSpec.swift	258	Lines should not have trailing whitespace.

Generated by 🚫 Danger

SD10 · 2017-08-16T20:08:48Z

Changelog.md

@@ -1,4 +1,5 @@
 # Next
+- Added tests for Single+Response and refactored Shared Test Helpers.


I don't know if adding tests needs a CHANGELOG entry but we usually list all API breaking changes first.

I don't mind removing this, I just saw a line saying
"Moved tests to Moya.xcodeproj." so wasn't sure if you usually include this sort of stuff there.

Should I move it to the bottom of breaking or get rid of it completely ?

I think moving it to the bottom would be great. Right under where we mention adding Single<Response>.

Sounds good to me ! 👍

freak4pc · 2017-08-16T20:22:01Z

Updated the Changelog and also did a "Build without Cache" for good luck 🍀

SD10 · 2017-08-16T20:29:44Z

@freak4pc I just cleared the cache and triggered a rebuild.
Anyways, thanks for taking care of this so Swiftly 😆 There's some nice cleanup in there.

freak4pc · 2017-08-16T20:30:53Z

Thanks for the review! @SD10
I'll wait for CI to clear and @sunshinejr to review this and feel free to merge whenever 👍

freak4pc · 2017-08-16T20:34:36Z

By the way this "cache" issue is probably the fault of scripts/bootstrap-if-needed.sh ...

SD10 · 2017-08-16T20:54:32Z

Tests/TestHelpers.swift

+#if os(OSX)
+import AppKit
+#else
+import Foundation


@freak4pc I think we have to import UIKit to use CGFloat 🤔

It was never there AFAIK. I think we need to import Foundation regardless of OS here.

Also didn't know the tests run on Xcode 9 / iOS 11 :) Haven't tried this locally. Will do a quick test and push a fix for this.

/Users/distiller/Moya/Tests/TestHelpers.swift:174:50: use of undeclared type 'CGFloat'; did you mean to use 'CGFloat'? -- This is such a weird CI error.

I learned yesterday that CGFloat is not declared in Foundation. It's part of UIKit 🤔

LOL, perfect error.

@SD10 GCFloat is actually imported in CoreGraphics, which in turn is not included in Foundation, but is included in UIKit. :)

@BasThomas I came to that realization after awhile! I have to stop thinking in the extreme that everything not in UIKit is Foundation. I originally thought that Foundation imported CoreGraphics because they have an Int.init(CGFloat)... Stopping to think -- it's actually more likely that CoreGraphics extends Foundation with this initializer... I'm such a noob 😆

https://developer.apple.com/documentation/swift/int/1455396-init
->
Framework
Core Graphics

Documentation is* awesome. 😬

* And with is, I mean can be.

Yeah that sort of is implicated in the "CG" prefix :)

By the way this is an even more interesting thing to know:
https://developer.apple.com/documentation/coregraphics/cgfloat/nativetype

codecov-io · 2017-08-16T21:37:20Z

Codecov Report

Merging #1229 into 9.0.0-dev will increase coverage by 2.75%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           9.0.0-dev    #1229      +/-   ##
=============================================
+ Coverage       76.4%   79.16%   +2.75%     
=============================================
  Files             24       24              
  Lines            763      763              
=============================================
+ Hits             583      604      +21     
+ Misses           180      159      -21

Impacted Files	Coverage Δ
Sources/RxMoya/Single+Response.swift	`100% <0%> (+100%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5cb8f6...96479a2. Read the comment docs.

sunshinejr · 2017-08-17T15:04:22Z

Changelog.md

@@ -3,6 +3,7 @@
 - **Breaking Change** Flattened `UploadType` and `DownloadType` into `Task` cases.
 - Added Swift 4.0 support.
 - Added all the `filter`/`map` operators that were available for `Observable<Response>` to `Single<Response>` as well.
+- Added tests for Single+Response and refactored Shared Test Helpers.


How about just:

Added tests for Single<Response> operators.

The refactor part might not be as important in my opinion.

sunshinejr · 2017-08-17T16:41:29Z

Tests/Single+MoyaSpec.swift

+            it("maps data representing an image to an image") {
+                let image = Image.testPNGImage(named: "testImage")
+                guard let data = image.asJPEGRepresentation(0.75) else {
+                    fail("Failed creating Data from Image")


I think that fatalError might better here, as it is not a failed test, but rather an error in preparation (this one is in few more places as well)

I actually had fatalError there and changed it to fail "on the last minute". I actually think both are fine but i'll change :)

sunshinejr · 2017-08-17T16:44:43Z

Tests/TestHelpers.swift

-        return self.addingPercentEncoding(withAllowedCharacters: .urlHostAllowed)!
-    }
-}
+#if os(OSX)


We may want to stick to more verbose conditional statements (as we had before):

#if os(iOS) || os(watchOS) || os(tvOS) ... #elseif os(OSX) ... #endif

sunshinejr · 2017-08-17T16:45:13Z

Tests/TestHelpers.swift

+        return Image(contentsOfFile: path!)!
+    }
+
+    #if os(iOS) || os(watchOS) || os(tvOS)


as we have here 😄

This contains it in ImageType and also cleared a Swiftlint warning.

sunshinejr · 2017-08-17T17:15:47Z

Changelog.md

@@ -7,6 +7,7 @@
 - Added Swift 4.0 support.
 - Added all the `filter`/`map` operators that were available for `Observable<Response>` to `Single<Response>` as well.
 - Added `AuthorizationType` to `AccessTokenAuthorizable` representing request headers of `.none`, `.basic`, and `.bearer`. 
+- Added tests for Single<Response> operators.


This is a minor one, but you forgot backticks around Single<Response> 😄

Fair enough ;)

sunshinejr

Thank you very much for these, @freak4pc 🎉

freak4pc changed the base branch from master to 9.0.0-dev August 16, 2017 19:43

freak4pc force-pushed the feature/rxmoya-single-tests branch from abec4a3 to 68ae475 Compare August 16, 2017 19:45

freak4pc requested review from sunshinejr and SD10 August 16, 2017 19:45

freak4pc mentioned this pull request Aug 16, 2017

[Tests] Add tests for Single+Response #1227

Closed

SD10 reviewed Aug 16, 2017

View reviewed changes

freak4pc force-pushed the feature/rxmoya-single-tests branch from 99619b7 to b50a2fe Compare August 16, 2017 20:18

SD10 reviewed Aug 16, 2017

View reviewed changes

SD10 approved these changes Aug 16, 2017

View reviewed changes

sunshinejr requested changes Aug 17, 2017

View reviewed changes

freak4pc added 7 commits August 17, 2017 19:52

Moved Mock Rx Response generators to RxTestHelpers

45e6b7c

Moved Image-related Test Helpers to TestHelpers

91a5865

Added test suite for Single+Response

f098ea5

Changed ImageJPEGRepresentation to ImageType.asJPEGRepresentation.

bafc9da

This contains it in ImageType and also cleared a Swiftlint warning.

Changed fatalError to a regular Quick fail to avoid force unwraps

1ac99b5

Apparently CGFloat and UIImageJPEGRepresentation are part of UIKit 🤔

c29d4ff

Moved Single+MoyaSpec into the Tests folder

1ef7cd3

freak4pc force-pushed the feature/rxmoya-single-tests branch 2 times, most recently from 7362f04 to 0b8c3c6 Compare August 17, 2017 16:53

sunshinejr reviewed Aug 17, 2017

View reviewed changes

Addressed PR comments from @sunshinejr

96479a2

freak4pc force-pushed the feature/rxmoya-single-tests branch from 0b8c3c6 to 96479a2 Compare August 17, 2017 17:17

sunshinejr approved these changes Aug 17, 2017

View reviewed changes

sunshinejr merged commit 486a933 into 9.0.0-dev Aug 17, 2017

sunshinejr deleted the feature/rxmoya-single-tests branch August 17, 2017 17:21

sunshinejr mentioned this pull request Sep 2, 2017

Formalize Changelog. #1260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for Single+Response and refactored Shared Test Helpers #1229

Added tests for Single+Response and refactored Shared Test Helpers #1229

freak4pc commented Aug 16, 2017 •

edited

Loading

MoyaBot commented Aug 16, 2017 •

edited

Loading

SD10 Aug 16, 2017

freak4pc Aug 16, 2017 •

edited

Loading

SD10 Aug 16, 2017

freak4pc Aug 16, 2017

freak4pc commented Aug 16, 2017

SD10 commented Aug 16, 2017

freak4pc commented Aug 16, 2017

freak4pc commented Aug 16, 2017

SD10 Aug 16, 2017

freak4pc Aug 16, 2017

freak4pc Aug 16, 2017 •

edited

Loading

SD10 Aug 16, 2017

freak4pc Aug 16, 2017

BasThomas Aug 17, 2017

SD10 Aug 17, 2017

BasThomas Aug 17, 2017

freak4pc Aug 17, 2017

freak4pc Aug 17, 2017

codecov-io commented Aug 16, 2017 •

edited

Loading

sunshinejr Aug 17, 2017

sunshinejr Aug 17, 2017 •

edited

Loading

freak4pc Aug 17, 2017

sunshinejr Aug 17, 2017 •

edited

Loading

sunshinejr Aug 17, 2017

sunshinejr Aug 17, 2017

freak4pc Aug 17, 2017

sunshinejr left a comment

		@@ -1,4 +1,5 @@
		# Next
		- Added tests for Single+Response and refactored Shared Test Helpers.

Added tests for Single+Response and refactored Shared Test Helpers #1229

Added tests for Single+Response and refactored Shared Test Helpers #1229

Conversation

freak4pc commented Aug 16, 2017 • edited Loading

MoyaBot commented Aug 16, 2017 • edited Loading

SwiftLint found issues

Warnings

Choose a reason for hiding this comment

freak4pc Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc commented Aug 16, 2017

SD10 commented Aug 16, 2017

freak4pc commented Aug 16, 2017

freak4pc commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freak4pc Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 16, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sunshinejr Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshinejr Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshinejr left a comment

Choose a reason for hiding this comment

freak4pc commented Aug 16, 2017 •

edited

Loading

MoyaBot commented Aug 16, 2017 •

edited

Loading

freak4pc Aug 16, 2017 •

edited

Loading

freak4pc Aug 16, 2017 •

edited

Loading

codecov-io commented Aug 16, 2017 •

edited

Loading

sunshinejr Aug 17, 2017 •

edited

Loading

sunshinejr Aug 17, 2017 •

edited

Loading