increase the test coverage by testing more mimetypes, and use some unicode characters
Details
Details
- Reviewers
mgallien bruns - Commits
- R286:ae3c38293421: increase test coverage of taglibwriter
taglibwriter test pass
Diff Detail
Diff Detail
- Repository
- R286 KFileMetaData
- Branch
- taglib_write_mimetypes
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 3308 Build 3326: arc lint + arc unit
Comment Actions
In general this looks good, but I would like two changes:
- Do the conversion to QTest first, and leave out the change for unicode testing (e.g. Title1 -> Title €)
- Add a third column like "stringsuffix", and then add another test (row) for each format. QStringLiteral("Title1") then becomes QStringLiteral("Title1") + stringsuffix
(2.) would go in a dependent review.
Comment Actions
Looks good to me. A nice and simple way to drastically increase test coverage.
| src/writers/taglibwriter.cpp | ||
|---|---|---|
| 22–24 | Unrelated. but consider making this static ? | |
| src/writers/taglibwriter.cpp | ||
|---|---|---|
| 22–24 | Never really though about that, but should be static, yes. None of the writers and extractors I looked at currently do this, so if I find time I will do that for all at once and separately | |
| autotests/taglibwritertest.cpp | ||
|---|---|---|
| 74 | Can you sort the tests alphabetically? It does not matter much, but IMHO it is nicer to have some rule for ordering, instead of arbitrary order. | |
| src/writers/taglibwriter.cpp | ||
| 25 | Can you also order these alphabetically based on the main mime type (aliases immediate after)? | |
| 30 | audio/wav and the next three are not backed by tests AFAICS, can you add these in a separate review, accompanied by tests files? | |