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

Tests: VLCLibrary #32

Closed
wants to merge 2 commits into from
Closed

Tests: VLCLibrary #32

wants to merge 2 commits into from

Conversation

mkchoi212
Copy link
Contributor

Needs to be rebased once #30 is merged

@mkchoi212 mkchoi212 force-pushed the test-lib branch 3 times, most recently from a108d95 to e13f1a4 Compare July 26, 2018 05:54
* libvlc will pass it to servers when required by protocol
* \param readableName human-readable application name, e.g. "FooBar player 1.2.3"
* \param readableName Human-readable application name, e.g. "FooBar player 1.2.3"
* \param userAgent HTTP User Agent, e.g. "FooBar/1.2.3 Python/2.6.0"
*/
- (void)setHumanReadableName:(NSString *)readableName withHTTPUserAgent:(NSString *)userAgent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not entirely sure how this can be tested...

* \param version application version numbers, e.g. "1.2.3"
* \param icon application icon name, e.g. "foobar"
* \param version Application version numbers, e.g. "1.2.3"
* \param icon Application icon name, e.g. "foobar"
*/
- (void)setApplicationIdentifier:(NSString *)identifier withVersion:(NSString *)version andApplicationIconName:(NSString *)icon;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing here

@mkchoi212 mkchoi212 force-pushed the test-lib branch 2 times, most recently from 604bd1d to 154b692 Compare August 3, 2018 08:18
for option in tests {
let library = try XCTAssertNotNilAndUnwrap(VLCLibrary(options: option))
XCTAssertNotNil(library.instance)
assertDefaultParameters()
Copy link
Member

@carolanitz carolanitz Aug 6, 2018

Choose a reason for hiding this comment

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

Wouldn't it be easier to understand and recognize why a test fails if you don't have a for loop and dict but write the params to test inline?
I know it's less code but right now I have to take some time to understand what line corresponds to a failing test if it would fail here. I'm also a bit unsure why we need to assertDefaultParams for : ["--no-video-title-show", "--verbose=4"]. If both of these are defaults it probably makes sense to test it as well with options that are not the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way to actually see if the custom options were successfully saved because the options are saved via

_instance = libvlc_new(count, lib_vlc_params);

where _instance is a void *.

So I just ended up making two dummy options and checking if the library was initialized successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I could init a mock libvlc_new(count, lib_vlc_params) and compare the two instances?? Is there a libvlc_compare_library method?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is but you can cast or bridge the void pointer into the correct object :)

(3, 3),
(4, 4),
(100, 0),
(-10, 0)
Copy link
Member

Choose a reason for hiding this comment

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

👍


XCTAssert(library.version.count > 1)
XCTAssert(library.compiler.count > 1)
XCTAssert(library.changeset.count > 1)
Copy link
Member

Choose a reason for hiding this comment

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

What you're testing for here is a bit unclear for me. Those are strings but the way the test is written it looks like you're testing an array. What do you think about testing for a not empty string and providing a message like "compiler should hold the string of the libvlccompiler, e.g. "gcc version 4.2.3 (Ubuntu 4.2.3-2ubuntu6)"? That way if the test fails we know roughly what we expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about doing

XCTAssertEqual(library.compiler, "gcc version 4.2.3 (Ubuntu 4.2.3-2ubuntu6))"

but every time the library gets an update, you would have to update the tests as well or else, they would fail. Is this desirable?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more along the lines of XCTAssertFalse (library.compiler. isEmpty, "compiler is empty but it should hold a string of the form e.g. "gcc version 4.2.3 (Ubuntu 4.2.3-2ubuntu6)")

let defaultParams = UserDefaults.standard.object(forKey: paramKey)

#if os(macOS)
XCTAssertEqual(defaultParams as? [String], expected)
Copy link
Member

Choose a reason for hiding this comment

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

When is defaultParams ever nil? Does it make sense to make this also not optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultParams is set in VLCLibrary during an init. But the user could override it by using the same key by mistake since the parameters are stored in UserDefaults.standard

Copy link
Member

Choose a reason for hiding this comment

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

I'm still confused a bit by this test :D so I overread the nil two lines down for iOS and I thought this test makes sure that the defaultParams are always not nil and set?

Copy link
Contributor Author

@mkchoi212 mkchoi212 Aug 10, 2018

Choose a reason for hiding this comment

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

defaultParams is nill for iOS and tvOS because VLCLibrary.m DOES NOT set the default parameters in the UserDefaults #if TARGET_OS_IPHONE. It does save the default params in UserDefaults otherwise.

#if TARGET_OS_IPHONE

XCTAssertTrue(library.debugLogging)

library.debugLogging = false
XCTAssertFalse(library.debugLogging)
Copy link
Member

Choose a reason for hiding this comment

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

👍


XCTAssertFalse(library.version.isEmpty, warn("3.0.4 Vetinari"))
XCTAssertFalse(library.compiler.isEmpty, warn("InstalledDir: /Applications/Xcode.app/..."))
XCTAssertFalse(library.changeset.isEmpty, warn("3.0.3-1-108-g7039639e6b"))
Copy link
Member

Choose a reason for hiding this comment

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

👍

@carolanitz
Copy link
Member

This was merged with 7734b08 & 55daaa9

@carolanitz carolanitz closed this Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants