Tests: Fix tempfile handling and disable broken MIME type detection check #4600

Holzhaus · 2022-01-05T15:07:47Z

Our tests left behind temporary files, and investigation revealed a bug in our business logic for determining the file type of an audio file. This PR fixes ~~both~~ the former and disables the test for the latter.

Related Zulip discussion: https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/git.20ignore.20generated.20test.20files.3F

daschuer

Thank you for taking care.
I think we should keep using the QMimeDatabase::MatchExtension see my comment below.

daschuer · 2022-01-05T16:13:00Z

src/sources/soundsource.cpp

@@ -48,7 +48,8 @@ QString SoundSource::getTypeFromFile(const QFileInfo& fileInfo) {
    const QString fileSuffix = fileInfo.suffix();
    const QString fileType = fileTypeFromSuffix(fileSuffix);
    DEBUG_ASSERT(!fileType.isEmpty() || fileType == QString{});
-    const QMimeType mimeType = QMimeDatabase().mimeTypeForFile(fileInfo);
+    const QMimeType mimeType = QMimeDatabase().mimeTypeForFile(
+            fileInfo, QMimeDatabase::MatchContent);


The control flow of this function is hard to understand. Along all the checks, it is hard to see what will take priority.
I think it should be:
1: MatchContent
2: MatchExtension
3: If we think we are smarter than MatchExtension use our fileType, which is essential just the suffix. (Maybe we can even drop it. It is just a hack around the "aif" issue, that might be already handled in QMimeDatabase

Is it correct, that may follows this pseudo code?

const QMimeType mimeType = QMimeDatabase().mimeTypeForFile( fileInfo, QMimeDatabase::MatchContent); if(mimeType.isDefault()) { mimeType = QMimeDatabase().mimeTypeForFile( fileInfo, QMimeDatabase::MatchExtension); if(mimeType.isDefault()) { mimeType = QMimeDatabase().mimeTypeForFile( fileInfo, QMimeDatabase::MatchExtension); } } const QString preferredSuffix = mimeType.preferredSuffix(); if (!preferredSuffix.isEmpty()) { return prefferedSuffix; } const QString fileSuffix = fileInfo.suffix(); retun preferredSuffixFromSuffix(fileSuffix); // old fileTypeFromSuffix

By the way I like the name "preferredSuffix" much more the fileType. It is hard to see what out propritary term "fileTtype" really is and the sound sources converts it back to suffix anyway any conversion function.

I have just verified, we can drop our custom aif aiff hack. QMimeDatabase::MatchExtension does it for us. At least in case of Linux. we need to check the others. Our test suite will discover it.

I suppose the method can be refactored in a follow-up PR. Initially, I only wanted to fix the temp file issue with our tests. The only reason why I touched this file at all in this PR is because fixing the leftover temp files revealed that the temp file names were also wrong, which in turn revealed a bug in our file type detection code (the test fails when the intended filenames are used).

The distinction was necessary because each "file type" in Mixxx (e.g. "aiff") may be associated with multiple "file suffixes". This originated from the wrong assumption of a 1:1 relation between file types and file suffixes before.

Please don't mix up these different concepts again!

@daschuer I agree with Jan, please do not extend the scope of this PR!

My proposal was just to rename "fileType" to "preferredSuffix" and rely on QMimeType.

Alternatively we may replace all FileTyp instances with QMimeTye and look up the preferredSuffix whenever needed.

What did you think?

uklotzde · 2022-01-05T16:45:34Z

#4601: Please reject, cherry-pick from or later rebase on this PR as desired.

uklotzde

Does this also affect 2.3? I guess so. Backport instead of targeting main?

Holzhaus · 2022-01-05T17:26:28Z

Does this also affect 2.3? I guess so. Backport instead of targeting main?

Oh right, I thought I checked that but somehow I thought it didn't. Will rebase on 2.3.

daschuer · 2022-01-05T17:29:21Z

I suppose the method can be refactored in a follow-up PR. Initially, I only wanted to fix the temp file issue with our tests.

This is a major change! Now we no longer rely on the file extension but on the file content wich may fail now files, that where playable before. We don't know how reliable the content based mime type detection really is.

If you like to touch the topic, you should also clean up the function for the new priority. If not, I would prefer to keep Qts default behaviour and fix the test instead.

daschuer · 2022-01-05T17:31:31Z

The mime type change is not suitable for a bugfix release, because of a big risk if regression.

daschuer · 2022-01-05T17:42:33Z

Qt uses conditionally zlib or zstd as mime provider. I don't know if the underlying database is the same for all our targets. This has at least potential to make the issue more complex.

The QTemporaryFile/QTemporaryDir classes are very convenient and fit our use cases well, because they can create a temporary file/dir that automatically gets deleted when the object goes out of scope. Instead of using these classes directly, we were using wrappers (namely `generateTemporaryFileName` and `createEmptyTemporaryFile`) that created a temp file and then just returned its name. This renders the QTemporaryFile auto-removal mechanism useless because the object goes out of scope before writing it. To mitigate this, we used a custom `FileRemover` class to work around the broken tempfile auto-removal. This is error-prone and we actually forgot to add it in some tests: https://mixxx.zulipchat.com/#narrow/stream/247620-development-help/topic/git.20ignore.20generated.20test.20files.3F Another major issue with the old API was that we often passed a *filename* to `generateTemporaryFileName` instead of a filename *template*. The distinction is important, because the latter may append random characters to the passed string: > If the file template contains XXXXXX that will automatically be > replaced with the unique part of the filename, otherwise a filename > will be determined automatically based on the static portion specified. > > Source: https://doc.qt.io/qt-5/qtemporaryfile.html#setFileTemplate Hence, the generated temporary file names weren't suitable for tests where the filename was important, such as SoundSourceProxyTest.getTypeFromFile where were didn't actually check wappens in cases where the file suffix is missing, misleading or empty, but unknowingly always checked the case that the suffix is unknown (i.e. instead of testing `file_with_no_file_suffix` we checked `file_with_no_file_suffix.HPqSHg`). This commit removes the APIs that break our tests and replaces them with QTemporaryDir/QTemporaryFile instead, so that the tests work as expected and don't leave temporary files behind.

Holzhaus · 2022-01-05T19:22:11Z

This is a major change! Now we no longer rely on the file extension but on the file content wich may fail now files, that where playable before.

If the mime type cannot be detected or if it has no preferred file extension, then we still use the original file extension for detecting the file type. The only way I can think of is if the QMimeDatabase detects the wrong mime type.

Before, the mime database was only used if there was no (known) extension. Now it's also used if there is a known extension, which should make files with the wrong extension playable. When you look at the unit test, you can see that this is the indended behavior:

mixxx/src/test/soundproxy_test.cpp

Lines 720 to 721 in 93c8980

    
           const QString filePathWithWrongSuffix = 
        
                   mixxxtest::generateTemporaryFileName("file_with_wrong_suffix.wav");

mixxx/src/test/soundproxy_test.cpp

Lines 744 to 746 in 93c8980

    
           EXPECT_STREQ(qPrintable("mp3"), 
        
                   qPrintable(mixxx::SoundSource::getTypeFromFile( 
        
                           filePathWithWrongSuffix)));

The only reason why this is slipped through is that our test case was buggy.

daschuer · 2022-01-05T21:40:11Z

I acknowledge that the desired behavior was not correctly implemented. However this is just IMHO a missing feature, changing the behavior for the Windows like extension-first that is also default for Qt to the Linux like content-first approach. It does not fix a bug and is IMHO not a 2.3 topic, because of the regression risk.

I can imagine that there are stream recordings that starts in the middle will by a chance detected wrong. The other issue might be ambiguous magic numbers, where we replace the right type with the preferredSuffix for a certain magic number. I think that this a minor downside compared to the benefit of playing files with incorrect extension, but we should not risk that in a bugfix update.

Here is by the way a description of the magic numbers used for this on Linux:
https://www.garykessler.net/library/file_sigs.html

I just did a test with an mp3 file renamed to wav and a wav file renamed to mp3. Both are playable with this patch and the correct type is shown in the type column according to the content. I think that is desire. Totem on Linux player does the same.

I have also tested a wav file without extension. This is still not playable, I think this is a bug. Playing such a file is IMHO desired.

What do you think? I would prefer to retarget the mime changes to main and merge is together with the pending issues.

daschuer

LGTM, Thank you.

Holzhaus added the minor bug label Jan 5, 2022

Holzhaus requested a review from uklotzde January 5, 2022 15:07

Holzhaus added this to the 2.4.0 milestone Jan 5, 2022

github-actions bot added the code quality label Jan 5, 2022

daschuer requested changes Jan 5, 2022

View reviewed changes

uklotzde mentioned this pull request Jan 5, 2022

Preserve complete file extension of temporary test files #4601

Closed

uklotzde reviewed Jan 5, 2022

View reviewed changes

Holzhaus force-pushed the test-fix-tempfile-handling branch from 1519cb7 to c609404 Compare January 5, 2022 19:05

Holzhaus changed the base branch from main to 2.3 January 5, 2022 19:05

Holzhaus modified the milestones: 2.4.0, 2.3.2 Jan 5, 2022

Holzhaus added the changelog This PR should be included in the changelog label Jan 5, 2022

Holzhaus force-pushed the test-fix-tempfile-handling branch from c609404 to ba1dfbc Compare January 5, 2022 19:14

test: Disable broken check in SoundProxyTest.getTypeFromFile

6690063

Holzhaus force-pushed the test-fix-tempfile-handling branch from ba1dfbc to 6690063 Compare January 5, 2022 21:59

Holzhaus changed the title ~~SoundSource: Fix wrong MIME type detection and fix tempfile handling tests~~ Tests: Fix tempfile handling and disable broken MIME type detection check Jan 5, 2022

Holzhaus removed minor bug changelog This PR should be included in the changelog labels Jan 5, 2022

Holzhaus mentioned this pull request Jan 5, 2022

Soundsource filetype detection fix #4602

Merged

uklotzde approved these changes Jan 5, 2022

View reviewed changes

Holzhaus requested a review from daschuer January 5, 2022 22:23

daschuer approved these changes Jan 6, 2022

View reviewed changes

daschuer merged commit 7bc495c into mixxxdj:2.3 Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests: Fix tempfile handling and disable broken MIME type detection check #4600

Tests: Fix tempfile handling and disable broken MIME type detection check #4600

Holzhaus commented Jan 5, 2022 •

edited

Loading

daschuer left a comment •

edited

Loading

daschuer Jan 5, 2022

daschuer Jan 5, 2022

daschuer Jan 5, 2022 •

edited

Loading

Holzhaus Jan 5, 2022

uklotzde Jan 5, 2022 •

edited

Loading

uklotzde Jan 5, 2022

daschuer Jan 5, 2022

uklotzde commented Jan 5, 2022

uklotzde left a comment

Holzhaus commented Jan 5, 2022

daschuer commented Jan 5, 2022

daschuer commented Jan 5, 2022

daschuer commented Jan 5, 2022

Holzhaus commented Jan 5, 2022 •

edited

Loading

daschuer commented Jan 5, 2022

daschuer left a comment

Tests: Fix tempfile handling and disable broken MIME type detection check #4600

Tests: Fix tempfile handling and disable broken MIME type detection check #4600

Conversation

Holzhaus commented Jan 5, 2022 • edited Loading

daschuer left a comment • edited Loading

Choose a reason for hiding this comment

daschuer Jan 5, 2022

Choose a reason for hiding this comment

daschuer Jan 5, 2022

Choose a reason for hiding this comment

daschuer Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Holzhaus Jan 5, 2022

Choose a reason for hiding this comment

uklotzde Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

uklotzde Jan 5, 2022

Choose a reason for hiding this comment

daschuer Jan 5, 2022

Choose a reason for hiding this comment

uklotzde commented Jan 5, 2022

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented Jan 5, 2022

daschuer commented Jan 5, 2022

daschuer commented Jan 5, 2022

daschuer commented Jan 5, 2022

Holzhaus commented Jan 5, 2022 • edited Loading

daschuer commented Jan 5, 2022

daschuer left a comment

Choose a reason for hiding this comment

Holzhaus commented Jan 5, 2022 •

edited

Loading

daschuer left a comment •

edited

Loading

daschuer Jan 5, 2022 •

edited

Loading

uklotzde Jan 5, 2022 •

edited

Loading

Holzhaus commented Jan 5, 2022 •

edited

Loading