Code review comment for lp:~ctf/checkbox/bug811177

> Hi Daniel,
> cpufreq-selector is in the package: gnome-applets which is not the
> default package in Oneiric.
> Actually in the cpu_scaling_test script, while
> setParameterWithSelector(using cpufreq-selector) failed, it will run
> setParameter(change governor using echo to sys file), so even we saw
> the cpufreq-selector error, it is still working.
> My patch is a fix to the problem that the cpufreq sys file location is
> changed in Oneiric, I use findParameter() to search the sys file, so
> it will backward compatible.

Hey, thanks for the backward compatibility bit! I saw that, very clever!

I offer my apologies, I tested this on a system that actually fails the test and I assumed the script to be at fault. Based on your reply I tested on another system that passes the test and it works fine, exiting with the correct "0" code when all tests pass. Governors are set correctly.

> I have reviewed the source code of cpufreq-selector, I think it's ok
> to make a checkbox-version cpufreq-selector, but since the
> cpu_scaling_test is working without cpufreq-selector(we may remove the
> error message).
> Do we still need that? Please let me know your idea, thanks.

No, as I mention it was my mistake, if cpufreq-selector is no longer needed then it's even better for us.

My only suggestion would be to fix it so, if cpufreq-selector is not available, this error message is not printed:

/bin/sh: cpufreq-selector: not found
Note: command failed: cpufreq-selector -g conservative

This is in the setParameterWithSelector method.

However, the test is working now, so the code could be merged as it is. Let me know if you agree with my suggestion or you would prefer I merge in the current state.

Thanks again! great job with this script.

>
> On Wed, Sep 21, 2011 at 3:57 AM, Daniel Manrique
> <email address hidden> wrote:
> > Review: Needs Information
> >
> > Hi Tim,
> >
> > I ran the script on a Natty system and it works fine, however on Oneiric it
> still fails due to absence of cpufreq-selector. When run, the test produces
> output as seen here:
> >
> > http://paste.ubuntu.com/693991/
> >
> > The exit code is 1, indicating failure (and as the output shows, "Error:
> performance setting vs minimum of 58.1% is not within 10.0% margin").
> >
> > Do you know of a way to replace cpufreq-selector? I think once we have that
> we can consider this fixed, but we're still missing that one part (even though
> the test now runs, it will fail most of the time).
> >
> > Thanks!
> >
> > --
> > https://code.launchpad.net/~ctf/checkbox/bug811177/+merge/75683
> > You are the owner of lp:~ctf/checkbox/bug811177.
> >
>
>
>
> --
> Tim