Improved unit tests for FFmpegRunner #1364
Conversation
…endent on an installed media player, leave junk on the screen, and leave orphaned temp files.
tombogle
force-pushed
the
improve-tests
branch
from
afd7bcb to
1642a26
Compare
LibPalaso Tests 36 files - 9 36 suites - 9 4m 34s ⏱️ - 7m 21s For more details on these failures, see this check. Results for commit 1642a26. ± Comparison against base commit de37405. This pull request removes 2353 tests. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @tombogle)
tombogle
force-pushed
the
improve-tests
branch
from
2146d15 to
1df0c03
Compare
Also cleaned up some outdated comments and did minor refactoring.
tombogle
force-pushed
the
improve-tests
branch
from
1df0c03 to
cf3c597
Compare
There was a problem hiding this comment.
Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hahn-kev and @tombogle)
| /// selects the test assemblies using Include="$(RootDir)/output/$(Configuration)/*.Tests.dll" which excludes exes. | ||
| /// I have not tried to verify that all of these tests would actually have problems on TeamCity, but it seemed | ||
| /// helpful to document in the usual way that they are not, in fact, run there. | ||
| /// </summary> |
There was a problem hiding this comment.
Seems like it is still worth a comment explaining why we are skipping these tests in CI.
There was a problem hiding this comment.
Actually this file never explained its dependency. Turns out that with that one test ignored, the only remaining test in the fixture runs just fine without any sound device enabled. So I'll remove the SkipOnTeamCity category. I'm now thinkingnthat doing something like this is the real answer:https://github.com/NathanCheshire/CyderUtils/actions/runs/9733272570/workflow. Then all these tests could run in the CI build.
…now able to use version of FFmpeg found on the path
| /// selects the test assemblies using Include="$(RootDir)/output/$(Configuration)/*.Tests.dll" which excludes exes. | ||
| /// I have not tried to verify that all of these tests would actually have problems on TeamCity, but it seemed | ||
| /// helpful to document in the usual way that they are not, in fact, run there. | ||
| /// </summary> |
There was a problem hiding this comment.
Actually this file never explained its dependency. Turns out that with that one test ignored, the only remaining test in the fixture runs just fine without any sound device enabled. So I'll remove the SkipOnTeamCity category. I'm now thinkingnthat doing something like this is the real answer:https://github.com/NathanCheshire/CyderUtils/actions/runs/9733272570/workflow. Then all these tests could run in the CI build.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hahn-kev and @tombogle)
tombogle
force-pushed
the
improve-tests
branch
from
20e6220 to
962ee5a
Compare
There was a problem hiding this comment.
Reviewed 16 of 16 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @hahn-kev and @tombogle)
CHANGELOG.md line 82 at r6 (raw file):
- [SIL.Windows.Forms.Archiving] Changed Cancel Button to say Close instead in IMDIArchivingDlg. - [SIL.Core.Desktop] Renamed GetFromRegistryProgramThatOpensFileType to GetDefaultProgramForFileType. - [SIL.Media] Made FFmpegRunner able to use version of FFmpeg found on the path.
Does the irrklang upgrade want a line here?
tombogle
force-pushed
the
improve-tests
branch
from
962ee5a to
b7e5120
Compare
|
Previously, andrew-polk wrote…
Done. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hahn-kev and @tombogle)
SIL.Media/FFmpegRunner.cs
Outdated
| return ffmpeg; | ||
| } | ||
|
|
||
| private static bool MeetsMinimumVersionRequirement(string ffmpeg) |
There was a problem hiding this comment.
are we concerned that this will execute every time (unless an explicit path is set) LocateFFmpeg is called? There's some places where it's called every time rather than using LocateAndRememberFFmpeg
There was a problem hiding this comment.
It looks like it has been that way for over a decade. Changing it would be a very subtle change. It's potentially a breaking change but only in the really bizarre edge case where an app does not install FFmpeg and the user installs it while running the app. In the grand scheme of things re-locating it each time probably isn't a huge performance penalty, but it seems pretty clear that in general this class was set up to check once and then assume the situation would not change. Maybe these methods were ported over from some app that didn't have this concept. I think I'll fix it. within sillsdev, the methods in question are used only in SayMore, unit tests and check (not sure what it was, but its repo is marked as deprecated).
…quiresAudioInputDevice to make it clear what requirement might not be met when tests fail. In FFmpegRunner, changed methods to use LocateAndRememberFFmpeg instead of LocateFFmpeg. Made the Windows implementation of ISimpleAudioSession attempt to create an irrKlang-based recorder even if there is no audio output device enabled. Improved some of the AudioSession unit tests to not require an audio input device if they are only testing playback. Made AudioRecorderTests fail at the fixture level if an audio input device is not present rather than throwing an obscure exception inside OnHandleCrated.
… (not sure why installing FFmpeg worked once and now can't be found on the path.)
|
I see now that there is separate logic for locating FFmpeg and FFprobe, and they are not in sync. I'll finish getting them on the same page tomorrow. |
…ation as FFmpeg and also on the system path). Made FFmpegRunner explicitly a static class (technically a breaking change, though all methods were already static). Made FFmpegRunner look for FFmpeg on the path before trying to find a version installed for Audacity (which is unlikely to succeed anyway). Renamed FFmpegRunner.FfmpegMinimumVersion property to MinimumVersion. Significant refactoring in FFmpegRunner and MediaInfo Made private utility functions in FFmpegRunner and MediaInfo into local functions so they would not accidentally be called by other methods in those classes. Added some test cases for MediaInfo.FFprobeFolder setter
tombogle
force-pushed
the
improve-tests
branch
from
4a8106a to
10d0165
Compare
…t require input/output devices
tombogle
force-pushed
the
improve-tests
branch
2 times, most recently
from
0eaa2e9 to
2a5390f
Compare
…and running installer directly.
tombogle
force-pushed
the
improve-tests
branch
from
2a5390f to
c1e1801
Compare
…ent. Excluding unit tests having the category RequiresAudioInputDevice
…o succeed consistently on Appveyor
… setup to succeed consistently on Appveyor" This reverts commit 7ec02ef.
With this change, these tests do not:
This change is