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
Setup testing environment #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smaller pullrequests will be a lot better. I simply couldn't check if you covered all the methods and all cases of it . Make one PR with all tests for one function :) Otherwise this will become a huuuge Pullrequest and I will miss way too much
GCC_C_LANGUAGE_STANDARD = gnu11; | ||
GCC_OPTIMIZATION_LEVEL = s; | ||
GCC_PRECOMPILE_PREFIX_HEADER = YES; | ||
GCC_PREFIX_HEADER = Headers/PCH/MobilePrefix.pch; | ||
GCC_SYMBOLS_PRIVATE_EXTERN = YES; | ||
HEADER_SEARCH_PATHS = "$(SRCROOT)/libvlc/vlc/include"; | ||
INSTALL_PATH = /usr/local/lib; | ||
IPHONEOS_DEPLOYMENT_TARGET = 7.0; | ||
IPHONEOS_DEPLOYMENT_TARGET = 8.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you changing the deployment target ?
CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR; | ||
CLANG_WARN_DOCUMENTATION_COMMENTS = YES; | ||
CLANG_WARN_EMPTY_BODY = YES; | ||
CLANG_WARN_ENUM_CONVERSION = YES; | ||
CLANG_WARN_INFINITE_RECURSION = YES; | ||
CLANG_WARN_INT_CONVERSION = YES; | ||
CLANG_WARN_NON_LITERAL_NULL_CONVERSION = YES; | ||
CLANG_WARN_OBJC_IMPLICIT_RETAIN_SELF = YES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this get added because you pressed the update xcode settings button ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think so. Should this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no that's fine from my side. I was just wondering
}]; | ||
} | ||
|
||
@end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
***************************************************************************** | ||
* Copyright (C) 2007 Pierre d'Herbemont | ||
* Copyright (C) 2013, 2017 Felix Paul Kühne | ||
* Copyright (C) 2007, 2013 VLC authors and VideoLAN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pierre and Felix didin't write this class ;)
|
||
func testInitializers() { | ||
let testTime: Int32 = 100000 | ||
let expected = TimeResult(value: NSNumber(value: testTime), string: "01:40", verboseString: "1 minutes 40 seconds", minuteString: "1", intValue: testTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have minute here
let expected = TimeResult(value: nil, string: "--:--", verboseString: "", minuteString: "", intValue: 0) | ||
expected.assertEqual(nullTime) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you missed testTimeWithNumber, testtimeWithInt and right ?
expected.assertEqual(nullTime) | ||
} | ||
|
||
func testInitializers() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be testInitWithNumber ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I combined tests for initWithNumber
and initwithInt
into tesIntializers
to make things less verbose. Should they be separated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. One test to test one thing or one method
expected.assertEqual(timeFromInt) | ||
expected.assertEqual(timeFromNumber) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testInitWithInt is missing
I split up the big commit into smaller chunks so it's easier to see! |
68d92b3
to
d3415d3
Compare
affd6fd
to
3507780
Compare
Tests for
VLCTime.m
have been added.Changes to
project.pbxproj
have been made to make the tests compile but may need some cleanup in the future.