-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add tests from taglib #51
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.
Just a few notes 🙂.
If you're still working on this, just wanted to let you know 1d5d397 should invalidate #51 (comment) for the most part. I noticed TagLib still produces different numbers, but far less frequently. |
It would probably be the best to implement something like assert with delta, something that is used for floats that permits ±1. |
@@ -74,6 +77,7 @@ fn test_save_id3v2() { | |||
tfile.insert_tag(tag); | |||
file.rewind().unwrap(); | |||
tfile.save_to(&mut file).unwrap(); | |||
assert!(!tfile.contains_tag_type(lofty::TagType::ID3v2)); |
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.
running 1 test
thread 'test_aiff::test_save_id3v2' panicked at 'assertion failed: !tfile.contains_tag_type(lofty::TagType::ID3v2)', tests/taglib/test_aiff.rs:80:9
stack backtrace:
0: rust_begin_unwind
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:584:5
1: core::panicking::panic_fmt
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:142:14
2: core::panicking::panic
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:48:5
3: taglib::test_aiff::test_save_id3v2
at ./tests/taglib/test_aiff.rs:80:3
4: taglib::test_aiff::test_save_id3v2::{{closure}}
at ./tests/taglib/test_aiff.rs:49:1
5: core::ops::function::FnOnce::call_once
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:248:5
6: core::ops::function::FnOnce::call_once
at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
test test_aiff::test_save_id3v2 ... FAILED
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.
But if I comment out this line similar test in line 90 runs successfully.
Lofty apparently keeps empty ID3v2 in it struct but does not write it, while TagLib probably removes it on save to keep internal struct in-sync with file.
Is this made on porpuse or is it a bug @Serial-ATA?
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.
Looks like TagLib removes them in File::save
. Its not purposeful, just didn't think of doing it that way. I'll add it soon 🙂.
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.
And something analogus to how TagLib handles files would be very useful too. The File instance would then needed to be stored somewhere inside.
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.
Do you mean tying a TaggedFile
to the File
it was read from? Something like that could be pretty easily accomplished with a wrapper. I could add that as BoundTaggedFile
maybe? I don't want that to be the default behavior though, to keeps things reusable.
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, something like that. I was also thinking about wrapper that would allowed that and provide backwards compatibility.
Hey @sagudev, do you mind if I take this over? 🙂 |
You may take this over; unfortunately it's been low on my priority list. |
Alright, thanks for your work on this! |
Currently test are on ignore as they are not working due to lofty.