Skip to content

Conversation

guykogus
Copy link
Contributor

@guykogus guykogus commented Jul 28, 2020

🚀
As this is our first XCTest extension it's a bit tricky to work out where everything goes, so this is mainly starting as a discussion.

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 5.0.
  • New extensions support iOS 10.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+, or use @available if not.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.
  • I have added a changelog entry describing my changes.

Issues

Targets

Source files that include XCTest can only be added to test targets.
This means that they can't be included in the frameworks in the Xcode project, which means that they can't be included in Carthage.
If a non-test target makes any reference to XCTest, even if it's protected by

#if canImport(XCTest)
import XCTest

it will throw a build-time error saying Failed to load module 'XCTest', so it will probably cause projects to fail building if we add it via CocoaPods and, probably, SPM.

Location

Where do the source files belong? If we place them in Sources/SwifterSwift/XCTest then they will be included in the Swift package, unless we add that folder to the excludes list, which is easy enough.
However, if we add it to Tests/XCTest then people may miss those extensions because they never open that folder. Maybe not 🤷‍♂️

Imports

In order to use Color in this example I need to call @testable import SwifterSwift, which may cause problems if we do find a way to make these extensions part of the framework. A solution would be to copy the typealiases.
Regardless, it's not really an issue unless we can actually include it in the library along with the other files anyway.

New project

An alternative is to actually have a separate project, let's say called SwifterTests, which has SwifterSwift as a dependency. It would allow anyone who's mad enough to include the entire project as a dependency to include SwifterSwift for their project and SwifterTests to their test targets. E.g.

target 'MyApp' do
   pod 'SwifterSwift'

   target 'MyAppTests' do
      pod 'SwifterTests'
   end
end

Obviously there are a bunch of issues here, the main one being visibility, but I think it's the cleanest option.

@guykogus guykogus mentioned this pull request Jul 28, 2020
7 tasks
@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #889 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #889   +/-   ##
=======================================
  Coverage   92.92%   92.92%           
=======================================
  Files         100      100           
  Lines        3448     3448           
=======================================
  Hits         3204     3204           
  Misses        244      244           
Flag Coverage Δ
#ios 95.67% <ø> (ø)
#macos 82.94% <ø> (ø)
#tvos 95.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 54b6eab...e74134b. Read the comment docs.

@SwifterSwiftBot
Copy link

SwifterSwiftBot commented Jul 28, 2020

4 Messages
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.
📖 iOS: Executed 692 tests, with 0 failures (0 unexpected) in 29.516 (32.161) seconds
📖 tvOS: Executed 654 tests, with 0 failures (0 unexpected) in 5.801 (6.985) seconds
📖 macOS: Executed 514 tests, with 0 failures (0 unexpected) in 5.364 (5.726) seconds

Generated by 🚫 Danger

@gurgeous
Copy link
Contributor

Just reading over your comment now... Since this is our first test extension, I'd probably vote for something cheap and cheerful:

  • put the file in Tests/XCTest
  • copy the typealiases so the file is standalone
  • encourage people to paste this file into their project
  • documentation

This is something we encourage with our extensions in general, right? Seems like a logical direction here, and sidesteps the build issues outlined above. If adoption is high and users are clamoring for a better solution, we can also investigate more complicated distribution mechanisms (Cocoapods, Carthage, SPM, etc)

@LucianoPAlmeida
Copy link
Member

@guykogus This still WIP? Let us know when it's ready
Ahh also, we might need to add a new subspec on pods file :)

@guykogus
Copy link
Contributor Author

guykogus commented Aug 2, 2020

@guykogus This still WIP? Let us know when it's ready
Ahh also, we might need to add a new subspec on pods file :)

I was waiting to hear feedback from yourself and @omaralbeik. It's ready based on the current limitations.
Also, if we add the new subspecpod lib lint breaks because of the error I mentioned: Failed to load module 'XCTest'

@guykogus guykogus changed the title [WIP] Add XCTest extension for comparing Color objects Add XCTest extension for comparing Color objects Aug 2, 2020
@guykogus
Copy link
Contributor Author

guykogus commented Aug 2, 2020

BTW I'm looking forward to Xcode 12 going live so that we can have a throwing version of this function.

@LucianoPAlmeida
Copy link
Member

I was waiting to hear feedback from yourself and @omaralbeik. It's ready based on the current limitations.

Ah sorry, I saw the WIP so was actually waiting too ... haha

Also, if we add the new subspecpod lib lint breaks because of the error I mentioned: Failed to load module 'XCTest'

Uhum, I've never run into this, but my guess is that we have to tell the subspec that it does need the XCTest das dependency framework like this
https://github.com/Quick/Nimble/blob/2c6978e887d7caa8acc2cab7b38e89d4d63eafb6/Nimble.podspec#L40
Have you test this?

Copy link
Member

@LucianoPAlmeida LucianoPAlmeida left a comment

Choose a reason for hiding this comment

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

Overall looks good to me! Just some minor points

@guykogus guykogus merged commit 46997c6 into master Aug 19, 2020
@guykogus guykogus deleted the xctest-color branch August 19, 2020 09:31
@guykogus guykogus mentioned this pull request Aug 21, 2020
nasircsms pushed a commit to nasircsms/SwifterSwift that referenced this pull request Aug 26, 2020
* 'master' of https://github.com/SwifterSwift/SwifterSwift: (79 commits)
  Helper functions for scrolling to the edges or by page of a scroll view (SwifterSwift#888)
  Add XCTest extension for comparing Color objects (SwifterSwift#889)
  Fix swiftlint error (SwifterSwift#894)
  Bump kramdown from 2.1.0 to 2.3.0 (SwifterSwift#891)
  Added note for UIView.addShadow() (SwifterSwift#890)
  loadFromNib(withClass) (SwifterSwift#885)
  constraint finders (widthConstraint, heightConstraint, leadingConstraint, trailingConstraint, topConstraint, bottomConstraint) (SwifterSwift#886)
  added String.regexEscaped (SwifterSwift#883)
  Make √ generic for FloatingPoint values (SwifterSwift#880)
  Update UITextField extensions (SwifterSwift#878)
  Added `masksToBounds` (IBInspectable) extension (SwifterSwift#877)
  Reinstate debounce function (SwifterSwift#869)
  `RandomAccessCollection`/`Collection` generalisations (SwifterSwift#863)
  Added HKActivitySummary extensions (SwifterSwift#875)
  Added new method for easily instantiating a view controller from a storyboard (SwifterSwift#860)
  Clean up code (SwifterSwift#864)
  fixed "".truncated crashed (SwifterSwift#866)
  Deprecated map(by:), compactMap(by:), filter(by:) (SwifterSwift#862)
  Overloaded 'contains' operator for string regex matching (SwifterSwift#858)
  Added convenient wrapper to asyncAfter (SwifterSwift#859)
  ...

# Conflicts:
#	Sources/SwifterSwift/AppKit/NSColorExtensions.swift
#	Sources/SwifterSwift/AppKit/NSImageExtensions.swift
#	Sources/SwifterSwift/AppKit/NSViewExtensions.swift
#	Sources/SwifterSwift/CoreGraphics/CGColorExtensions.swift
#	Sources/SwifterSwift/CoreGraphics/CGFloatExtensions.swift
#	Sources/SwifterSwift/CoreGraphics/CGPointExtensions.swift
#	Sources/SwifterSwift/CoreGraphics/CGSizeExtensions.swift
#	Sources/SwifterSwift/Foundation/NSAttributedStringExtensions.swift
#	Sources/SwifterSwift/SwiftStdlib/StringExtensions.swift
#	SwifterSwift.xcodeproj/project.pbxproj
#	Tests/AppKitTests/NSColorExtensionsTests.swift
#	Tests/SwiftStdlibTests/StringExtensionsTests.swift
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.

4 participants