Skip to content
This repository was archived by the owner on Sep 19, 2018. It is now read-only.

Define setup_default_test_properties. #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

calvaris
Copy link

@calvaris calvaris commented Apr 15, 2015

This allows a set of tests to share a common set of properties without
having to pass all the tests the same options.


This change is Reviewable

This allows a set of tests to share a common set of properties without
having to pass all the tests the same options.
@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/4687

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@jgraham
Copy link
Member

jgraham commented Apr 15, 2015

Out of interest, do you have an example of a test where you used this?

@calvaris
Copy link
Author

Sure, https://bugs.webkit.org/attachment.cgi?id=250787&action=prettypatch

Here, I would gladly use that function instead of passing the parameters to all tests

@jgraham
Copy link
Member

jgraham commented Apr 15, 2015

So {timeout:foo} is really deprecated since it just tends to add instability compared to running the test to the full harness timeout. I think I should really just remove support. Do you have some reason for using it here?

@calvaris
Copy link
Author

Yes. Every WebKit test file has its own timeout and we have several testharness tests in each file. If for example the first testharness test times out, it can mean (and it happens in some cases) that the whole test file is marked as timeout, when what we want is to have the granularity of the testharness timing out (or not).

@jgraham
Copy link
Member

jgraham commented Apr 15, 2015

Sorry, I still don't understand the issue.

If you don't specify a timeout, that test will get marked as TIMEOUT when the overall page times out (or, if a step function is never called, as NOT RUN). The only thing that adding a per-test timeout can do is to reduce this timeout which just has the effect of adding more instability (which is bad). You can still tell which subtests timed out. If you are taking a status of TIMEOUT for the overall page to be more problematic than a status of OK but with subtests timed out, it sounds like your integration with testharness.js has the wrong semantics.

@jgraham
Copy link
Member

jgraham commented Apr 15, 2015

Oh, maybe the case you are worried about is when you try to sequence the tests, even though they are async, but some time out. If you really need to do that (do you?) your scheduler can implement a fixed timeout for each test and call test.force_timeout() Then you only need to specify the timeout in one place. But it's a rather worrying thing to do (because, for example, some of the hardware Mozilla run tests on is quite slow and it is very possible to miss 150ms timeouts, so the test that you linked before would likely get disabled or edited due to instability in any case).

@calvaris
Copy link
Author

Yes, I know that reducing the timeout can lead to a bit more of instability, which shouldn't be a big trouble at least in the case that I am dealing with. Even the slowest bots can cope with 150ms, at least most of the times and even if you get small degree of flakyness, it is better than having a whole test file which several tests timing out with no information of which test caused the problem, which is my real issue. In the WebKit environment, when a test file times out, you don't get information of what happened inside the test, just that it timed out so you don't get the granularity of testnarness timeouts.

About your second comment, yes, I can do that, but my intention is trying to avoid per test configuration. I don't care if I have to specify per test timeout or per test timeout, it is a problem.

Our intention is to reduce the general testharness timeout and if there is any problem with a certain test set, we can increase it for those cases.

Besides, this pull request has only to do with timeouts in a collateral way, as I posting a way of customize all tests parameters at once, which in our special case, it is the timeout which we discussed above.

@calvaris
Copy link
Author

If you're not interested in the PR and do not want to discuss it any further, please close it or let me know to do it myself.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants