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

Added unit test to read incomplete mp3 file; added incomplete mp3 file. #64

Merged
merged 3 commits into from
Mar 10, 2018

Conversation

codealchemist
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage remained the same at 91.042% when pulling bf6d60f on codealchemist:unit-test-incomplete-mp3 into e7c78fb on Borewit:master.

@Borewit
Copy link
Owner

Borewit commented Mar 7, 2018

Thanks again for your thorough feedback. I don’t have my development tools around for a few days, I will look into it when I arrive home.

Just a few things I notice. If your intend is to simulate a partial downloaded file, it is read as stream not as a file. I assume the stream stops at the first “gap”. I don’t think the ID3v1 header can be read this way, because these are the last 128 bytes of the stream/file.

@codealchemist
Copy link
Contributor Author

I saw what you mean.
It's parseStream vs. parseFile.

Related comment here:
webtorrent/webtorrent-desktop#1240

Thanks!

Borewit added a commit that referenced this pull request Mar 9, 2018
@Borewit
Copy link
Owner

Borewit commented Mar 9, 2018

To give this test a better home then ID3v1, I created a generic MP3 parsing file: test-mp3.ts. Can you please move the test in there?

Alberto I think we should either use the file parser for partial downloaded file (as you have done in your test now), or do additional tests with streams based on partial downloaded file, in such a way that it reflects the webtorrent-desktop context.

@codealchemist
Copy link
Contributor Author

Sure, I'll move the test there.
Thx!

I think both cases, a partially downloaded file, and an unfinished stream, are valid use cases.
Might be good to test both.
What do you think?

@Borewit Borewit merged commit cc9755d into Borewit:master Mar 10, 2018
@Borewit
Copy link
Owner

Borewit commented Mar 10, 2018

I think both cases, a partially downloaded file, and an unfinished stream, are valid use cases.
Might be good to test both.
What do you think?

In principle I agree it could be valid use cases, adding a disclaimer to the partial downloaded files, hard to predict the exact behavior. The file is broken after all.

I don't know what the best strategy is for webtorrent-desktop, I suggest we address this as a separate issue, together with an improved update mechanism for the metadata, which is in my opinion related to this.

Therefor I opened a new issue to capture this: webtorrent/webtorrent-desktop#1340

@codealchemist
Copy link
Contributor Author

Sounds good!
Thanks!

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.

None yet

3 participants