158476 2016-06-07 05:06:39 -0700 [GTK] Use a longer timeout for glib unit tests 2016-06-07 23:48:18 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Local Build Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply lforschler mcatanzaro oldest_to_newest 1200008 0 cgarcia 2016-06-07 05:06:39 -0700 The timeout is supposed to be per test case, but in the case of GLib tests it affects all the tests cases of the same test program. Some test programs like TestLoaderClient, that have a lot of test cases, often time out in the bots because the timeout is not enough to run all the tests cases. So, we should use a longer timeout for GLib tests (for example, timeout * 2). 1200009 1 280695 cgarcia 2016-06-07 05:08:34 -0700 Created attachment 280695 Patch 1200027 2 280695 mcatanzaro 2016-06-07 07:08:11 -0700 Comment on attachment 280695 Patch I don't like this hack; we should instead mark affected tests as SLOW. 1200127 3 cgarcia 2016-06-07 10:55:12 -0700 (In reply to comment #2) > Comment on attachment 280695 [details] > Patch > > I don't like this hack; we should instead mark affected tests as SLOW. This is not a hack, we are using the same timeout value to measure different things. At the beginning, all google tests were run like GLib tests, the binary was run with all the tests, but we changed that and we run the same binary with the a filter for every test case. That's possible with google tests because there's a way to get a list of test cases-. That's not possible for GLib tests, and now we have the same timeout value that is supposed to be a test case timeout used for tests cases in case of google and test suites in case of GLib. It's not that TestLoaderClient is slow, and not that we are using a higher value for slow tests, it's just that it runs a lot of test cases. The timout for a single tests case is usually enough to run all TestLoaderClient tests cases, but not always, which means that in the bots it sometimes times out, and it sometimes passes. So it makes sense to use to use a higher timeout for GLib tests and 20 instead of 10 should be enough and still short enough. 1200136 4 mcatanzaro 2016-06-07 11:34:09 -0700 (In reply to comment #3) > That's not > possible for GLib tests Can't we use 'gtester -p'? >, and now we have the same timeout value that is > supposed to be a test case timeout used for tests cases in case of google > and test suites in case of GLib. It's not that TestLoaderClient is slow, and > not that we are using a higher value for slow tests, it's just that it runs > a lot of test cases. The timout for a single tests case is usually enough to > run all TestLoaderClient tests cases, but not always, which means that in > the bots it sometimes times out, and it sometimes passes. So it makes sense > to use to use a higher timeout for GLib tests and 20 instead of 10 should be > enough and still short enough. Yes, I understand, but I don't think a hardcoded, arbitrary 2 is a good way to handle this. We're going to have to find that in a couple years once a test program requires even longer to run. Marking the affected tests as SLOW seems cleaner to me. Anyway, I didn't give r- so you can push ahead if you still disagree. 1200356 5 cgarcia 2016-06-07 23:41:50 -0700 (In reply to comment #4) > (In reply to comment #3) > > That's not > > possible for GLib tests > > Can't we use 'gtester -p'? You can run tests cases independently with -p, but as I said you can't get the list of tests cases to run, so the test script would need to know in advance the name of all tests cases for all test suites. > >, and now we have the same timeout value that is > > supposed to be a test case timeout used for tests cases in case of google > > and test suites in case of GLib. It's not that TestLoaderClient is slow, and > > not that we are using a higher value for slow tests, it's just that it runs > > a lot of test cases. The timout for a single tests case is usually enough to > > run all TestLoaderClient tests cases, but not always, which means that in > > the bots it sometimes times out, and it sometimes passes. So it makes sense > > to use to use a higher timeout for GLib tests and 20 instead of 10 should be > > enough and still short enough. > > Yes, I understand, but I don't think a hardcoded, arbitrary 2 is a good way > to handle this. We're going to have to find that in a couple years once a > test program requires even longer to run. Marking the affected tests as SLOW > seems cleaner to me. Anyway, I didn't give r- so you can push ahead if you > still disagree. All timeout values we use are arbitrary, so the 10 seconds timeout we use for every specific test case is as arbitrary as using 20 for tests suites. If we mark TestLoaderClient as a slow now (which is not slow at all), we will have the same problem eventually if we add more tests cases to other tests, so using a different timeout for test suites than for tests cases also prevents this from happening. So, yes, I still disagree. 1200359 6 cgarcia 2016-06-07 23:48:18 -0700 Committed r201795: <http://trac.webkit.org/changeset/201795> 280695 2016-06-07 05:08:34 -0700 2016-06-07 08:58:43 -0700 Patch wk-glib-test-timeout.diff text/plain 2030 cgarcia ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCBjNmUy MGFjLi4yNGY4NzU1IDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMTYtMDYtMDcgIENhcmxvcyBHYXJjaWEgQ2FtcG9z ICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCisgICAgICAgIFtHVEtdIFVzZSBhIGxvbmdlciB0aW1l b3V0IGZvciBnbGliIHVuaXQgdGVzdHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE1ODQ3NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFRoZSB0aW1lb3V0IGlzIHN1cHBvc2VkIHRvIGJlIHBlciB0ZXN0IGNh c2UsIGJ1dCBpbiB0aGUgY2FzZSBvZiBHTGliIHRlc3RzIGl0IGFmZmVjdHMgYWxsIHRoZSB0ZXN0 cyBjYXNlcyBvZiB0aGUKKyAgICAgICAgc2FtZSB0ZXN0IHByb2dyYW0uIFNvbWUgdGVzdCBwcm9n cmFtcyBsaWtlIFRlc3RMb2FkZXJDbGllbnQsIHRoYXQgaGF2ZSBhIGxvdCBvZiB0ZXN0IGNhc2Vz LCBvZnRlbiB0aW1lIG91dCBpbgorICAgICAgICB0aGUgYm90cyBiZWNhdXNlIHRoZSB0aW1lb3V0 IGlzIG5vdCBlbm91Z2ggdG8gcnVuIGFsbCB0aGUgdGVzdHMgY2FzZXMuIFNvLCB3ZSBzaG91bGQg dXNlIGEgbG9uZ2VyIHRpbWVvdXQgZm9yCisgICAgICAgIEdMaWIgdGVzdHMuCisKKyAgICAgICAg KiBTY3JpcHRzL3J1bi1ndGstdGVzdHM6CisgICAgICAgIChUZXN0UnVubmVyLl9ydW5fdGVzdF9n bGliKToKKwogMjAxNi0wNi0wNiAgQ2FybG9zIEFsYmVydG8gTG9wZXogUGVyZXogIDxjbG9wZXpA aWdhbGlhLmNvbT4KIAogICAgICAgICBSRUdSRVNTSU9OKHIyMDE0NDkpIFtHVEtdIEFSTXY3IGJ1 aWxkIGZhaWxzIHdpdGggbGliaWN1ZGF0YS5zby41NTogY2Fubm90IG9wZW4gc2hhcmVkIG9iamVj dCBmaWxlIG9uIGd0a2RvYy1zY2FuZ29iaiBzdGVwLgpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0 cy9ydW4tZ3RrLXRlc3RzIGIvVG9vbHMvU2NyaXB0cy9ydW4tZ3RrLXRlc3RzCmluZGV4IGMxOGZh MjMuLjA5ODA3MzYgMTAwNzU1Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvcnVuLWd0ay10ZXN0cworKysg Yi9Ub29scy9TY3JpcHRzL3J1bi1ndGstdGVzdHMKQEAgLTMyNiw3ICszMjYsMTEgQEAgY2xhc3Mg VGVzdFJ1bm5lcjoKICAgICAgICAgZm9yIHRlc3RfY2FzZSBpbiBzZWxmLl90ZXN0X2Nhc2VzX3Rv X3NraXAodGVzdF9wcm9ncmFtKToKICAgICAgICAgICAgIHRlc3Rlcl9jb21tYW5kLmV4dGVuZChb Jy1zJywgdGVzdF9jYXNlXSkKICAgICAgICAgdGVzdGVyX2NvbW1hbmQuYXBwZW5kKHRlc3RfcHJv Z3JhbSkKLSAgICAgICAgdGltZW91dCA9IHNlbGYuX29wdGlvbnMudGltZW91dAorICAgICAgICAj IFRoaXMgdGltZW91dCBpcyBzdXBwb3NlZCB0byBiZSBwZXIgdGVzdCBjYXNlLCBidXQgaW4gdGhl IGNhc2Ugb2YgR0xpYiB0ZXN0cyBpdCBhZmZlY3RzIGFsbCB0aGUgdGVzdHMgY2FzZXMgb2YKKyAg ICAgICAgIyB0aGUgc2FtZSB0ZXN0IHByb2dyYW0uIFNvbWUgdGVzdCBwcm9ncmFtcyBsaWtlIFRl c3RMb2FkZXJDbGllbnQsIHRoYXQgaGF2ZSBhIGxvdCBvZiB0ZXN0IGNhc2VzLCBvZnRlbiB0aW1l IG91dAorICAgICAgICAjIGluIHRoZSBib3RzIGJlY2F1c2UgdGhlIHRpbWVvdXQgaXMgbm90IGVu b3VnaCB0byBydW4gYWxsIHRoZSB0ZXN0cyBjYXNlcy4gU28sIHdlIHVzZSBhIGxvbmdlciB0aW1l b3V0IGZvciBHTGliCisgICAgICAgICMgdGVzdHMgKHRpbWVvdXQgKiAyKS4KKyAgICAgICAgdGlt ZW91dCA9IHNlbGYuX29wdGlvbnMudGltZW91dCAqIDIKICAgICAgICAgdGVzdCA9IG9zLnBhdGgu am9pbihvcy5wYXRoLmJhc2VuYW1lKG9zLnBhdGguZGlybmFtZSh0ZXN0X3Byb2dyYW0pKSwgb3Mu cGF0aC5iYXNlbmFtZSh0ZXN0X3Byb2dyYW0pKQogICAgICAgICBpZiB0ZXN0IGluIFRlc3RSdW5u ZXIuU0xPVzoKICAgICAgICAgICAgIHRpbWVvdXQgKj0gNQo=