Skip to content

Tests: VLCMediaList #35

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

Closed
wants to merge 3 commits into from
Closed

Conversation

mkchoi212
Copy link
Contributor

@mkchoi212 mkchoi212 commented Aug 1, 2018

Includes tests for #34 VLCMediaList::removeMediaAt
Needs to be rebased after #34

@mkchoi212 mkchoi212 force-pushed the medialist-tests branch 2 times, most recently from 955b24e to c84b345 Compare August 3, 2018 06:21
let oldCount = mediaList.count

// Remove media from list
let ok = mediaList.removeMedia(at: idx)
Copy link
Member

Choose a reason for hiding this comment

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

move the XCTAssertTrue here maybe? :D

@mkchoi212 mkchoi212 force-pushed the medialist-tests branch 2 times, most recently from 6d77660 to 69b65e9 Compare August 27, 2018 01:36
@mkchoi212
Copy link
Contributor Author

Commits have been rebased :D Sorry for noticing it just now!


for (deleteIdx, count, ok) in tests {
let removalResult = mediaList.removeMedia(at: deleteIdx)
XCTAssertEqual(removalResult, ok)
Copy link
Member

Choose a reason for hiding this comment

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

I would rename it from ok to removalSuccessful

(10, 0, false),
]

let source = Video.standards.map{ VLCMedia(path: $0.path) }
Copy link
Member

Choose a reason for hiding this comment

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

isn't this fragile? When someone adds a Video to the standards the entire tests will fail, right?

class VLCMediaListTest: XCTestCase {

func testRemoveMedia() throws {
let videos = [Video.test1, Video.test1, Video.test2, Video.test3, Video.test4, Video.test4]
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach better than the standards

]

for (targetIdx, expectedState, ok) in tests {
let removalResult = mediaList.removeMedia(at: targetIdx)
Copy link
Member

Choose a reason for hiding this comment

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

👍

(1, [NF, 0,NF,NF,NF, 1], true),
(8, [NF, 0,NF,NF,NF, 1], false),
(0, [NF,NF,NF,NF,NF, 0], true),
(0, [NF,NF,NF,NF,NF,NF], true),
Copy link
Member

Choose a reason for hiding this comment

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

quick question when you try again to remove at index 0 after removing it already at index 0 shouldn't the result be false? Do we need the NF in expectedState? I found it a bit confusing to match the index with the correct position because of it

Copy link
Contributor Author

@mkchoi212 mkchoi212 Aug 29, 2018

Choose a reason for hiding this comment

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

quick question when you try again to remove at index 0 after removing it already at index 0 shouldn't the result be false?

No, because the previous state contains a media. expectedState is what the list looks like after the removal.

Do we need the NF in expectedState?

I see where you are coming from and yeah it's a bit hard to match the indices. But I think it would be better to be explicit about exactly which media was removed.

e.g. Let say you have something like this

let videos = [Video.test1, Video.test1]
let removalResult = mediaList.removeMedia(at: 0)
let currentState = source.map{ mediaList.index(of: $0) }
XCTAssertEqual(currentState, [0])

From XCTAssertEqual(currentState, [0]), you can't tell which of the two Video.test1 was removed.

@mkchoi212 mkchoi212 force-pushed the medialist-tests branch 2 times, most recently from fc2efed to f7d8d71 Compare August 29, 2018 16:40
@Mikanbu Mikanbu closed this Jun 17, 2024
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.

5 participants