Closed Bug 1826010 Opened 3 years ago Closed 2 years ago

Intermittent comm/mail/test/browser/folder-display/browser_deletionWithMultipleDisplays.js | This test exceeded the timeout threshold.

Categories

(Thunderbird :: Folder and Message Lists, defect, P5)

Product:
Thunderbird
Component:
Folder and Message Lists
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
115 Branch

People

(Reporter: intermittent-bug-filer, Assigned: john)

Details

(Keywords: intermittent-failure, intermittent-testcase)

Attachments

(1 file)

Bug 1826010 - Fix browser_deletionWithMultipleDisplays.js by doubling timeout for CCov runs to reflect the fact that they need twice as long. r=#thunderbird-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Assignee: nobody → john
Status: NEW → ASSIGNED
Attachment #9333278 - Attachment description: WIP: Bug 1826010 - Fix browser_deletionWithMultipleDisplays.js by doubling timeout for CCov runs to reflect the fact that they need twice as long. r=#thunderbird-reviewers → Bug 1826010 - Fix browser_deletionWithMultipleDisplays.js by doubling timeout for CCov runs to reflect the fact that they need twice as long. r=#thunderbird-reviewers
Keywords: checkin-needed-tb
Target Milestone: --- → 115 Branch

Pushed by john@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/c242cac88196
Fix browser_deletionWithMultipleDisplays.js by doubling timeout for CCov runs to reflect the fact that they need twice as long. r=leftmostcat

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

Out of curiosity, now I know there are methods to request longer time out.
One is the method used here to request a longer time out from within the test JS code.

The other method is setting a timeout multiplication factor in corresponding xpcshell.ini file...
Oh wait, this is a mochitest test.

So for a test executed as part of mochitest, requesting longer timeout from within the JS code is the only method to request a longer time out due to some execution idiosyncrasies such as CCOV, isn't it?

I am asking because I have tried to cope with the need of longer timeout for valgrind-run (much slower even than CCOV run. Usually close to x10 or more slower than the normal run.) by changing the timeout from runtests.py. But it has not worked well although it worked rather well during |make mozmill| test days. Yes, I did change the timeout by changing some test code files back then.

So if the use of "2" as in
requestLongerTimeout(AppConstants.MOZ_CODE_COVERAGE ? 2 : 1);
good for CCOV run, what should I use for |AppConstants.MOZ_CODE_COVERAGE| to cope with Valgrind run.

Hmm...

https://searchfox.org/comm-central/source/mozilla/remote/cdp/test/browser/head.js#25
shows

function getTimeoutMultiplier() {
  if (
    AppConstants.DEBUG ||
    AppConstants.MOZ_CODE_COVERAGE ||
    AppConstants.ASAN ||
    AppConstants.TSAN
  ) {
    return 4;
  }

  return 1;
}

It seems that valgrind run is not considered at all. :-(
valgrind has helped me to find memory-related issues over the years. It is a pity it is not addressed very well.
I wonder how firefox valgrind test run (I read that is done regularly) is handling the timeout issues.

Now I realize that there are places where AppConstans.MOZ_CODE_COVERAGE is referenced.
https://searchfox.org/comm-central/search?q=AppConstants.MOZ_CODE_COVERAGE&path=&case=false&regexp=false

MOZ_CODE_COVERAGE is defined here.
https://searchfox.org/comm-central/source/mozilla/toolkit/modules/AppConstants.sys.mjs#387

Maybe I should define MOZ_VALGRIND_RUN or something like that here,
and see if the modification of getTimeoutMultiplier as follows will help.

function getTimeoutMultiplier() {
  if (AppConstants.MOZ_VALGRIND_RUN) {
     return 20;   // <----- large factor for valgruind run
  }
  if (
    AppConstants.DEBUG ||
    AppConstants.MOZ_CODE_COVERAGE ||
    AppConstants.ASAN ||
    AppConstants.TSAN
  ) {
    return 4;
  }

  return 1;
}

Wait, the patch proposed here takes care of CCOV run, but it does NOT take into account ASAN or TSAN run, so maybe we should instead
use this |getTimeoutMultiplier()| instead? (I have sometimes run xpcshell/mochitest in the past using ASAN-enabled binaries).

I am not sure how the files are included, etc. and so the scope of the function is not very clear to me. So I cannot propose how to call the function, |getTimeoutMultipler()|, from each test.

https://searchfox.org/comm-central/source/mozilla/remote/cdp/test/browser/head.js#22
defines
const TIMEOUT_MULTIPLIER = getTimeoutMultiplier();
So it has the same scope issue, but we may want to include |TIMEOUT_MULTIPLER| instead.

In any case, looking at this patch makes me to try to proceed with valgrind timeout issue from the modification of each test, rather than
from the modification of runtests.py, and I suspect (maybe too optimistically) a change of single function |getTimeoutMultiplier()| may go a long way.
(And I still wonder how Firefox manages longer time out under valgrind, if the valgrind test is executed regularly.)

Edit: some typos were fixed. References to |TIMOUT_MULTIPLER| added.

Flags: needinfo?(john)

The relevant code for mochitests is here:

Define standard timeout:
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/browser-test.js#8

Which is overwritten here:
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/browser-test.js#84

And adjusted for special runs here (if I understood this code correctly):
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/runtests.py#2452-2493

The pref is set here for a couple of special runs (debug, asan, tsan), which is why the default debug timeout is 90s and not 45s.

So there is already some code which extends the timeout for slow runs, but valgrind is not one of them.

The thing with CCOV is that I have seen runs which do not need twice as long. But for this test it does. So I figured adjusting the timeout only for my tests would be better.

I do not know what is the best in your case. You could add a general rule for valgrind in the shown code section, or extend the timeout only for your tests. I do not know much about unit/xpcshell tests.

Flags: needinfo?(john)

(In reply to John Bieling (:TbSync) from comment #10)

The relevant code for mochitests is here:

Define standard timeout:
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/browser-test.js#8

Which is overwritten here:
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/browser-test.js#84

And adjusted for special runs here (if I understood this code correctly):
https://searchfox.org/mozilla-central/rev/eb07633057d66ab25f9db4c5900eeb6913da7579/testing/mochitest/runtests.py#2452-2493

The pref is set here for a couple of special runs (debug, asan, tsan), which is why the default debug timeout is 90s and not 45s.

So there is already some code which extends the timeout for slow runs, but valgrind is not one of them.

The thing with CCOV is that I have seen runs which do not need twice as long. But for this test it does. So I figured adjusting the timeout only for my tests would be better.

I do not know what is the best in your case. You could add a general rule for valgrind in the shown code section, or extend the timeout only for your tests. I do not know much about unit/xpcshell tests.

Thank you for the relevant code sections.
I think I would try to add general rule for valgrind in the show sections. The code in runtests.py look so promising.

I will report back, but please do not hold your breath. It took me almost a year before I could run |make mozmil| successfully under valgrind when I hacked on the code now and then sporadically.

Happy Hacking!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: