quick_shader: Do not exclude tests in sanity profile.

Submitted by Rafael Antognolli on Oct. 3, 2018, 5:51 p.m.

Details

Message ID 20181003175128.2728-1-rafael.antognolli@intel.com
State New
Headers show
Series "quick_shader: Do not exclude tests in sanity profile." ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Rafael Antognolli Oct. 3, 2018, 5:51 p.m.
We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
tests, but we want to keep at least the ones present in the sanity
profile. This makes the 'quick' and 'gpu' profiles a superset of the
'sanity' profile.
---
 tests/quick_shader.py | 3 +++
 1 file changed, 3 insertions(+)

Patch hide | download patch | download mbox

diff --git a/tests/quick_shader.py b/tests/quick_shader.py
index f1bda6545..dd8853cde 100644
--- a/tests/quick_shader.py
+++ b/tests/quick_shader.py
@@ -26,6 +26,7 @@  import random
 
 from framework import grouptools
 from .shader import profile as _profile
+from .sanity import profile as _sanity
 
 __all__ = ['profile']
 
@@ -42,6 +43,8 @@  class FilterVsIn(object):
 
     def __call__(self, name, _):
         if 'vs_in' in grouptools.split(name):
+            if name in _sanity.test_list:
+                return True
             # 20%
             return self.random.random() <= .2
         return True

Comments

Does this change the set of tests in the profile? I think it will since we'll
end up calling self.random.random() a different number of times.

Quoting Rafael Antognolli (2018-10-03 10:51:28)
> We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
> tests, but we want to keep at least the ones present in the sanity
> profile. This makes the 'quick' and 'gpu' profiles a superset of the
> 'sanity' profile.
> ---
>  tests/quick_shader.py | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/quick_shader.py b/tests/quick_shader.py
> index f1bda6545..dd8853cde 100644
> --- a/tests/quick_shader.py
> +++ b/tests/quick_shader.py
> @@ -26,6 +26,7 @@ import random
>  
>  from framework import grouptools
>  from .shader import profile as _profile
> +from .sanity import profile as _sanity
>  
>  __all__ = ['profile']
>  
> @@ -42,6 +43,8 @@ class FilterVsIn(object):
>  
>      def __call__(self, name, _):
>          if 'vs_in' in grouptools.split(name):
> +            if name in _sanity.test_list:
> +                return True
>              # 20%
>              return self.random.random() <= .2
>          return True
> -- 
> 2.17.1
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/piglit
On Fri, Oct 05, 2018 at 11:11:19AM -0700, Dylan Baker wrote:
> Does this change the set of tests in the profile? I think it will since we'll
> end up calling self.random.random() a different number of times.

That's the idea. Now a test always gets included if it's already in the
sanity profile. And self.random.random() won't be called for that test.

We currently only have one 'vs_in' test in the sanity profile, so this
change will only add potentially one new test to the quick_shader
profile.

On the other hand, checking if the test is in the sanity profile will be
done for every single 'vs_in' test now, so generating the list will be a
little slower.

> Quoting Rafael Antognolli (2018-10-03 10:51:28)
> > We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
> > tests, but we want to keep at least the ones present in the sanity
> > profile. This makes the 'quick' and 'gpu' profiles a superset of the
> > 'sanity' profile.
> > ---
> >  tests/quick_shader.py | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tests/quick_shader.py b/tests/quick_shader.py
> > index f1bda6545..dd8853cde 100644
> > --- a/tests/quick_shader.py
> > +++ b/tests/quick_shader.py
> > @@ -26,6 +26,7 @@ import random
> >  
> >  from framework import grouptools
> >  from .shader import profile as _profile
> > +from .sanity import profile as _sanity
> >  
> >  __all__ = ['profile']
> >  
> > @@ -42,6 +43,8 @@ class FilterVsIn(object):
> >  
> >      def __call__(self, name, _):
> >          if 'vs_in' in grouptools.split(name):
> > +            if name in _sanity.test_list:
> > +                return True
> >              # 20%
> >              return self.random.random() <= .2
> >          return True
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Piglit mailing list
> > Piglit@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/piglit
Quoting Rafael Antognolli (2018-10-05 11:25:24)
> On Fri, Oct 05, 2018 at 11:11:19AM -0700, Dylan Baker wrote:
> > Does this change the set of tests in the profile? I think it will since we'll
> > end up calling self.random.random() a different number of times.
> 
> That's the idea. Now a test always gets included if it's already in the
> sanity profile. And self.random.random() won't be called for that test.

So it's going to change the set of tests we run in CI? That seems like something
we should run by Clayton before we merge.

> We currently only have one 'vs_in' test in the sanity profile, so this
> change will only add potentially one new test to the quick_shader
> profile.
> 
> On the other hand, checking if the test is in the sanity profile will be
> done for every single 'vs_in' test now, so generating the list will be a
> little slower.
> 
> > Quoting Rafael Antognolli (2018-10-03 10:51:28)
> > > We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
> > > tests, but we want to keep at least the ones present in the sanity
> > > profile. This makes the 'quick' and 'gpu' profiles a superset of the
> > > 'sanity' profile.
> > > ---
> > >  tests/quick_shader.py | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/tests/quick_shader.py b/tests/quick_shader.py
> > > index f1bda6545..dd8853cde 100644
> > > --- a/tests/quick_shader.py
> > > +++ b/tests/quick_shader.py
> > > @@ -26,6 +26,7 @@ import random
> > >  
> > >  from framework import grouptools
> > >  from .shader import profile as _profile
> > > +from .sanity import profile as _sanity
> > >  
> > >  __all__ = ['profile']
> > >  
> > > @@ -42,6 +43,8 @@ class FilterVsIn(object):
> > >  
> > >      def __call__(self, name, _):
> > >          if 'vs_in' in grouptools.split(name):
> > > +            if name in _sanity.test_list:
> > > +                return True
> > >              # 20%
> > >              return self.random.random() <= .2
> > >          return True
> > > -- 
> > > 2.17.1
> > > 
> > > _______________________________________________
> > > Piglit mailing list
> > > Piglit@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/piglit
> 
>
On Fri, Oct 05, 2018 at 12:16:53PM -0700, Dylan Baker wrote:
> Quoting Rafael Antognolli (2018-10-05 11:25:24)
> > On Fri, Oct 05, 2018 at 11:11:19AM -0700, Dylan Baker wrote:
> > > Does this change the set of tests in the profile? I think it will since we'll
> > > end up calling self.random.random() a different number of times.
> > 
> > That's the idea. Now a test always gets included if it's already in the
> > sanity profile. And self.random.random() won't be called for that test.
> 
> So it's going to change the set of tests we run in CI? That seems like something
> we should run by Clayton before we merge.

Ugh, I see what you mean. Yeah, I'm not sure the change is worth it.

Maybe as an alternative, we could just call random() to that test too,
even though we discard the result. Or maybe just drop the whole patch
altogether...

> > We currently only have one 'vs_in' test in the sanity profile, so this
> > change will only add potentially one new test to the quick_shader
> > profile.
> > 
> > On the other hand, checking if the test is in the sanity profile will be
> > done for every single 'vs_in' test now, so generating the list will be a
> > little slower.
> > 
> > > Quoting Rafael Antognolli (2018-10-03 10:51:28)
> > > > We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
> > > > tests, but we want to keep at least the ones present in the sanity
> > > > profile. This makes the 'quick' and 'gpu' profiles a superset of the
> > > > 'sanity' profile.
> > > > ---
> > > >  tests/quick_shader.py | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/tests/quick_shader.py b/tests/quick_shader.py
> > > > index f1bda6545..dd8853cde 100644
> > > > --- a/tests/quick_shader.py
> > > > +++ b/tests/quick_shader.py
> > > > @@ -26,6 +26,7 @@ import random
> > > >  
> > > >  from framework import grouptools
> > > >  from .shader import profile as _profile
> > > > +from .sanity import profile as _sanity
> > > >  
> > > >  __all__ = ['profile']
> > > >  
> > > > @@ -42,6 +43,8 @@ class FilterVsIn(object):
> > > >  
> > > >      def __call__(self, name, _):
> > > >          if 'vs_in' in grouptools.split(name):
> > > > +            if name in _sanity.test_list:
> > > > +                return True
> > > >              # 20%
> > > >              return self.random.random() <= .2
> > > >          return True
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > _______________________________________________
> > > > Piglit mailing list
> > > > Piglit@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/piglit
> > 
> >
Quoting Rafael Antognolli (2018-10-05 12:44:02)
> On Fri, Oct 05, 2018 at 12:16:53PM -0700, Dylan Baker wrote:
> > Quoting Rafael Antognolli (2018-10-05 11:25:24)
> > > On Fri, Oct 05, 2018 at 11:11:19AM -0700, Dylan Baker wrote:
> > > > Does this change the set of tests in the profile? I think it will since we'll
> > > > end up calling self.random.random() a different number of times.
> > > 
> > > That's the idea. Now a test always gets included if it's already in the
> > > sanity profile. And self.random.random() won't be called for that test.
> > 
> > So it's going to change the set of tests we run in CI? That seems like something
> > we should run by Clayton before we merge.
> 
> Ugh, I see what you mean. Yeah, I'm not sure the change is worth it.
> 
> Maybe as an alternative, we could just call random() to that test too,
> even though we discard the result. Or maybe just drop the whole patch
> altogether...

I have no problem just calling self.random.random() before check if the name is
in the sanity list. It's a build-time cost anyway. If you go that route make
sure there's a comment :)

Dylan

> 
> > > We currently only have one 'vs_in' test in the sanity profile, so this
> > > change will only add potentially one new test to the quick_shader
> > > profile.
> > > 
> > > On the other hand, checking if the test is in the sanity profile will be
> > > done for every single 'vs_in' test now, so generating the list will be a
> > > little slower.
> > > 
> > > > Quoting Rafael Antognolli (2018-10-03 10:51:28)
> > > > > We randomly exclude 80% of the 'arb_vertex_attrib_64bit/execution/vs_in'
> > > > > tests, but we want to keep at least the ones present in the sanity
> > > > > profile. This makes the 'quick' and 'gpu' profiles a superset of the
> > > > > 'sanity' profile.
> > > > > ---
> > > > >  tests/quick_shader.py | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/tests/quick_shader.py b/tests/quick_shader.py
> > > > > index f1bda6545..dd8853cde 100644
> > > > > --- a/tests/quick_shader.py
> > > > > +++ b/tests/quick_shader.py
> > > > > @@ -26,6 +26,7 @@ import random
> > > > >  
> > > > >  from framework import grouptools
> > > > >  from .shader import profile as _profile
> > > > > +from .sanity import profile as _sanity
> > > > >  
> > > > >  __all__ = ['profile']
> > > > >  
> > > > > @@ -42,6 +43,8 @@ class FilterVsIn(object):
> > > > >  
> > > > >      def __call__(self, name, _):
> > > > >          if 'vs_in' in grouptools.split(name):
> > > > > +            if name in _sanity.test_list:
> > > > > +                return True
> > > > >              # 20%
> > > > >              return self.random.random() <= .2
> > > > >          return True
> > > > > -- 
> > > > > 2.17.1
> > > > > 
> > > > > _______________________________________________
> > > > > Piglit mailing list
> > > > > Piglit@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/piglit
> > > 
> > > 
> 
>