Add screen requirement annotation to reftest

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
7 years ago
7 years ago

People

(Reporter: BenWa, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Using ignoreWindowSize is not an option for GL Layers on android because we get both unexpected pass and fails for == and != tests respectively.

This suggestion is to add an annotation (is that the correct term?) to the reftest.list manifest to specify that the test allows a smaller resolution. For example resolution(300,200) would indicate that this test can be ran on devices with screens 300x200 or larger. No annotation would indicate that the standard 800x100 window size is required.

This change would allow us to start marking test that support smaller resolution incrementally.

Example:
resolution(300,200) fails-if(!haveTestPlugin) == plugin-sanity.html div-sanity.html
(Reporter)

Comment 1

7 years ago
Created attachment 562260 [details] [diff] [review]
Part 1: Add screeen(minWidth, minHeight) property
(Reporter)

Comment 2

7 years ago
Created attachment 562261 [details] [diff] [review]
Part 2: Add screen() property to reftest manifest files
(Reporter)

Comment 3

7 years ago
I wrote a simple script to get a weak upper bound for the screen resolution. Perhaps we can make improvements on the technique. It doesn't work for svg test that just report 800x1000 but seems to work well for most others. This should be a good starting point.

Tests that don't fit in the current resolution will be skipped. I will try this on an small android device Monday.

Are we interested in adding this annotation to reftests?
(In reply to Benoit Girard (:BenWa) from comment #3)
> Are we interested in adding this annotation to reftests?

Yes, definitely. I don't think it should be required, though; if reftests are very small, for example, we needn't include an annotation.

I think we should annotate only large reftests, and perhaps add some test to our reftest framework that will error out if it's suspected a test will go beyond a common phone screen size without an annotation.
(Reporter)

Comment 5

7 years ago
Created attachment 562755 [details] [diff] [review]
Get Window Size

Here's the patch I'm using to get the window size. I don't plan on checking this in anymore but I'm posting it in case it's useful for others.

Usage: 
EXTRA_TEST_ARGS="--get-screen-requirement" <REFTEST-CMD> | tee Runlog.txt
cat Runlog.txt | grep ScriptScreen > script.sh
Run 's/ScriptScreen: //g' on script.sh then run the script
(Reporter)

Comment 6

7 years ago
I've discuss this with Joe some more and we agree that the behavior we want is as follow:
- Test can use a screen resolution of 480x480 without any request. This covers all 'hdpi' phone screens (including Nexus S) and all tablet screens in any orientation: 
http://developer.android.com/guide/practices/screens_support.html
- Test that require above 480x480 will need to request a large screen. They can add 'screen(width,height)' to request additional space. This test will be executed to catching crashing bugs but will not cause an unexpected failure.

I'm going to post a patch for the revised reftest modification. Once that is approved, to reduce bit rot, I will post a patch to add the screen annotation to all test requiring >480x480.

Comment 7

7 years ago
(In reply to Benoit Girard (:BenWa) from comment #6)
> I've discuss this with Joe some more and we agree that the behavior we want
> is as follow:
> - Test can use a screen resolution of 480x480 without any request. This
> covers all 'hdpi' phone screens (including Nexus S) and all tablet screens
> in any orientation: 
> http://developer.android.com/guide/practices/screens_support.html
> - Test that require above 480x480 will need to request a large screen. They
> can add 'screen(width,height)' to request additional space. This test will
> be executed to catching crashing bugs but will not cause an unexpected
> failure.
> 
> I'm going to post a patch for the revised reftest modification. Once that is
> approved, to reduce bit rot, I will post a patch to add the screen
> annotation to all test requiring >480x480.
I want to make sure I'm following you.  So, if I have a test that uses 800x1000 I'd have to explicitly annotate that.  If I don't annotate anything then my test will be assumed to work within 480x480?
Correct. We hope to get a separate reftest suite running at 480x480 to catch tests that break when that isn't the case.

The alternative is to have unmarked tests be assumed to be "too big," which will lead to an ever-decreasing percentage of tests actually run on handhelds.
(In reply to Joe Drew (:JOEDREW!) from comment #8)
> Correct. We hope to get a separate reftest suite running at 480x480 to catch
> tests that break when that isn't the case.

That isn't sufficient -- many tests will continue to pass when run at 480x480, but not still continue to be testing what they were intended to be testing.  The most obvious reason for this will be that the failing part no longer fits in the 480x480 image, but there could certainly be others.
100% true, but we're not trying to solve all the problems with this annotation, only some of them.
(Reporter)

Comment 11

7 years ago
Since we're running the tegra at a sufficient resolution and developers can run the test with a tablet I'm no longer interested in adding this. It would of been nice to test on device with smaller screen but ohh well :(
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.