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: VLCAudio #24

Closed
wants to merge 3 commits into from
Closed

Tests: VLCAudio #24

wants to merge 3 commits into from

Conversation

mkchoi212
Copy link
Contributor

This PR is just here to show you guys what I have been working on :D.
I've written couple of comments here and there to illustrate what I did to fix the bugs I found.

I will split up the massivcommit/PR into separate PRs next week

@@ -217,7 +217,7 @@ typedef NS_ENUM(NSUInteger, VLCMediaType) {
* equivalent in lexical value, and NSOrderedDescending if the URL of the
* receiver follows media. If media is nil, returns NSOrderedDescending.
*/
- (NSComparisonResult)compare:(VLCMedia *)media;
- (NSComparisonResult)compare:(nullable VLCMedia *)media;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In VLCMedia.m::compare, the line

if (!media)

can not be satisfied unless the parameter is nullable.
Or can it without it??

@@ -256,6 +256,7 @@ typedef NS_ENUM(unsigned, VLCMediaParsedStatus)
VLCMediaParsedStatusInit = 0,
VLCMediaParsedStatusSkipped,
VLCMediaParsedStatusFailed,
VLCMediaParsedStatusTimeout,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[self setVolume: tempVolume];
}

- (void)volumeDown
{
int tempVolume = [self volume] - VOLUME_STEP;
if (tempVolume > VOLUME_MAX)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This predicate will never execute as setVolume performs this check already

@@ -122,17 +122,13 @@ - (void)volumeUp
int tempVolume = [self volume] + VOLUME_STEP;
if (tempVolume > VOLUME_MAX)
tempVolume = VOLUME_MAX;
else if (tempVolume < VOLUME_MIN)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This predicate is also impossible as setVolume already checks this

options,
-1);
// Using the default time-out value
return [self parseWithOptions:options timeout:-1];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the same thing as the deleted code :D

@@ -882,7 +886,9 @@ - (void)initInternalMediaDescriptor
return;
}

urlString = [urlString stringByAddingPercentEscapesUsingEncoding:NSUTF8StringEncoding];
if ([[urlString stringByRemovingPercentEncoding] isEqualToString:urlString]) {
Copy link
Contributor Author

@mkchoi212 mkchoi212 Jul 6, 2018

Choose a reason for hiding this comment

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

- (void)parsedChanged:(NSNumber *)isParsedAsNumber
{
[self willChangeValueForKey:@"parsedStatus"];
[self parsedStatus];
[self fetchEssentialMetaInformation];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1015,21 +1036,11 @@ - (void)setStateAsNumber:(NSNumber *)newStateAsNumber
[self setState: [newStateAsNumber intValue]];
}

#if TARGET_OS_IPHONE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why were there two separate metaDictionary functions for iOS and non-iOS platforms?

@mkchoi212 mkchoi212 changed the title Massive work in progress Tests: VLCAudio Jul 11, 2018
func XCTAssertNotNilAndUnwrap<T>(_ variable: T?, message: String = "Unexpected nil variable", file: StaticString = #file, line: UInt = #line) throws -> T {
guard let variable = variable else {
XCTFail(message, file: file, line: line)
throw UnexpectedNilError()
Copy link
Member

Choose a reason for hiding this comment

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

do we need to throw here since this is just a test ?

audio.isMuted = false
XCTAssertFalse(audio.isMuted)

audio.setMute(true)
Copy link
Member

Choose a reason for hiding this comment

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

huh, I wonder if we really need the setter and the property. @fkuehne ?

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed since this is a deprecated API

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this API will be removed in v4.

} else {
XCTAssertEqual(audio.volume, Int32(max - (i * step)))
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This code block is actually quite a bit confusing. I'm guessing 9000 is above the max ?
and what does the step do ? I have actually no clue what you're testing for :D

}
}

XCTAssertEqual(audio.volume, Int32(max))
Copy link
Member

Choose a reason for hiding this comment

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

same here as above


for (repeatCount, expected) in tests {
(0..<repeatCount).forEach { _ in audio.volumeDown() }
XCTAssertEqual(audio.volume, expected)
Copy link
Member

Choose a reason for hiding this comment

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

I just see this now but I think it might make sense to split setting volume and volumedown. Because right now you rely on audio.volume = Int32(9000) to cap and work so that the next tests succeed. They should be independent.

@carolanitz
Copy link
Member

merged with ca81ecd and following

@carolanitz carolanitz closed this Jul 20, 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
3 participants