Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for isEmpty instead of count > 0 #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rudrankriyam
Copy link

Hi there!

For this following test case, do you think it makes sense to check for isEmpty vs count > 0, so it is similar to the naming of the test?

@TomFern
Copy link

TomFern commented Aug 30, 2021

@rudrankriyam The change breaks the build. See error below:

Compiling TowerTests.swift 01:05
314❌ /Users/semaphore/semaphore-demo-ios-swift-xcode/TallestTowersTests/TowerTests.swift:27:15: cannot convert value of type 'Bool' to expected argument type 'Int' 01:05
315 XCTAssert(!Tower.tallestTowers.isEmpty > 0) 01:05
316 ^ 01:05
317Testing failed: 01:05
318 Command MergeSwiftModule failed with a nonzero exit code 01:05
319 Cannot convert value of type 'Bool' to expected argument type 'Int' 01:05
320 Testing cancelled because the build failed. 01:05
321** TEST FAILED ** 01:05
322The following build commands failed: 01:05
323 MergeSwiftModule normal x86_64 01:05
324 CompileSwiftSources normal x86_64 com.apple.xcode.tools.swift.compiler 01:05
325 CompileSwift normal x86_64 /Users/semaphore/semaphore-demo-ios-swift-xcode/TallestTowersTests/TowerTests.swift 01:05
326(3 failures)

https://semaphore-demos.semaphoreci.com/jobs/3badb246-dfa8-4a5f-b2e9-198404ed0862

@rudrankriyam
Copy link
Author

I'm not sure why the test is merging both the changes into one, i.e. XCTAssert(!Tower.tallestTowers.isEmpty > 0)

My suggested change is XCTAssert(!Tower.tallestTowers.isEmpty)

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.

2 participants