Issue metadata
Sign in to add a comment
|
AudioDebugRecording*Tests are flaky |
||||||||||||||||||||||
Issue descriptionAudioDebugRecordingSessionImplTest.CreateFileCreatesExpectedFiles failed in fuchsia/x64 FYI build https://build.chromium.org/p/chromium.fyi/builders/Fuchsia/builds/13690, with: [ RUN ] AudioDebugRecordingSessionImplTest.CreateFileCreatesExpectedFiles ../../media/audio/audio_debug_recording_session_impl_unittest.cc:144: Failure Value of: base::PathExists(output_recording_filename) Actual: false Expected: true [ FAILED ] AudioDebugRecordingSessionImplTest.CreateFileCreatesExpectedFiles (246 ms) AudioDebugRecordingHelperTest.EnableDisable failed in Fuchsia/x64/Debug FYI build https://build.chromium.org/p/chromium.fyi/builders/Fuchsia%20%28dbg%29/builds/15958 with: [ RUN ] AudioDebugRecordingHelperTest.EnableDisable ../../media/audio/audio_debug_recording_helper_unittest.cc:160: Failure Value of: base::DeleteFile(file_path, false) Actual: false Expected: true [ FAILED ] AudioDebugRecordingHelperTest.EnableDisable (143 ms) By inspection, the EnableDisable test is flakey, since it passes a base::File to a be deleted on a background sequence, but calls base::DeleteFile on it on the main thread. Although these flakes were observed on the Fuchsia FYI bots, forcing a delay in the deletion of the AudioDebugFileWriter, e.g. by adding a Sleep() in its destructor, should cause the issue to repro reliably on any platform[1]. These changes were made in https://chromium-review.googlesource.com/881501 -> assigning to marinaciocea@chromium.org. [1] Though note that some platforms allow files to be deleted while they are open, while others do not. :)
,
Feb 7 2018
Issue is that the tests are all using the same filenames, so when they are batched and run in parallel by TestLauncher they risk clashing.
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/afaf7a45dc8b51c7c13a3ac2105eef4b4fabc949 commit afaf7a45dc8b51c7c13a3ac2105eef4b4fabc949 Author: Wez <wez@chromium.org> Date: Wed Feb 07 02:52:58 2018 Fix AudioDebugRecording*Tests to use isolated temporary directories. https://chromium-review.googlesource.com/881501 introduced fixed file paths into some of these tests, causing them to randomly interfere with one another if they happened to end up being run in parallel. Bug: 809780 TBR: olka@chromium.org Change-Id: Idb2143f9c44ddab559b7402182e69d3b2e7220de Reviewed-on: https://chromium-review.googlesource.com/905991 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#534893} [modify] https://crrev.com/afaf7a45dc8b51c7c13a3ac2105eef4b4fabc949/media/audio/audio_debug_recording_helper_unittest.cc [modify] https://crrev.com/afaf7a45dc8b51c7c13a3ac2105eef4b4fabc949/media/audio/audio_debug_recording_session_impl_unittest.cc
,
Feb 7 2018
Marina: As previously noted, I think the new test code is making an invalid assumption about the AudioDebugFileWriter being synchronous (see the comment before the DeleteFile call), when it actually works on a background SequencedTaskRunner - WDYT? The immediate issue of filename clashes is dealt with, so removing the Fuchsia-blocking label. :)
,
Feb 7 2018
P.S. If you're confident that the test threading assumptions are valid then feel free to close this out as Fixed!
,
Feb 8 2018
Sorry Wez, I missed your previous note. Actually AudioDebugFileWriter runs on the main thread, its inner class AudioDebugFileWriter::AudioFileWriter however runs on a background task runner. The two classes have very similar names, maybe that's a bit confusing. In the test MockAudioDebugFileWriter::Start is called synchronously from |reply_callback| which is bound to AudioDebugRecordingHelper::StartDebugRecordingToFile, and AudioDebugFileWriter::AudioFileWriter is not created[1] because MockAudioDebugFileWriter::Start is mocked. [1] https://cs.chromium.org/chromium/src/media/audio/audio_debug_file_writer.cc?rcl=a4d8757543b2aab668a1f2e380ae12b9989c2e8b&l=274 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by w...@chromium.org
, Feb 6 2018