Merge lp://qastaging/~fginther/ubuntu-test-cases/add-test-suite-timeout into lp://qastaging/ubuntu-test-cases/touch

Proposed by Francis Ginther
Status: Merged
Merged at revision: 419
Proposed branch: lp://qastaging/~fginther/ubuntu-test-cases/add-test-suite-timeout
Merge into: lp://qastaging/ubuntu-test-cases/touch
Diff against target: 16 lines (+5/-1)
1 file modified
scripts/run-autopilot-tests.sh (+5/-1)
To merge this branch: bzr merge lp://qastaging/~fginther/ubuntu-test-cases/add-test-suite-timeout
Reviewer Review Type Date Requested Status
Para Siva (community) Needs Information
Review via email: mp+281155@code.qastaging.launchpad.net

Commit message

Use 'TEST_TIMEOUT' to add a timeout to the test suite execution.

Description of the change

Use 'TEST_TIMEOUT' to add a timeout to the test suite execution.

To post a comment you must log in.
Revision history for this message
Para Siva (psivaa) wrote :

Thanks Francis for working on it.
I'm in two minds for this change, just a little reluctant for a change at this level at this point in time.

1. With `|| true` being used with phablet-test-run for various other reasons, it does not give us an east way to print a statement to state that timeout has occurred. Without an indication that the failure is due to timeout, it will be harder for someone to debug.
2. Also, traditionally, the absence of the subunit file due to timed out job is reflected by a failed Jenkins job rather than an unstable one. Although an unstable job is the right one, this is a deviation from the past.
3. Upon a timeout [1] the test_result.xml only states that there is one failure despite the job having many actual test failures (whilst leaving the job unstable). So I dont think it does not improve the reporting.
4. It will be impossible to select the timeout value. A timeout that will be enough today may become unsuitable tomorrow for many reasons, one of which is that the number of tests in that app. The app team may add more tests to the suite making the timeout not enough.

I'm fine for merging this if you still think this is change worth making :)

[1]: http://s-jenkins.ubuntu-ci:8080/job/psiva-generic-deb-autopilot-runner-vivid-touch/5/

review: Needs Information
Revision history for this message
Para Siva (psivaa) wrote :

One thing that I agree that is that its upto the users to use this feature or not. If they chose not to use it then it is as it is now. If they chose it then they have to be mindful of these deviations

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches