Re: [msi/tests 3/3] Create a system restore point when needed

[James Hawkins]

On Sat, Aug 30, 2008 at 3:14 PM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote:
> Hi,
>
> The slowness on some machines for the install tests comes from the system
> restore facility. When 'System Restore' is active you will find a great
> number
> of system restore points after these tests have run. Adding our own system
> restore point circumvents this as Windows will now wait till we tell it that
> the
>  system change has ended. Only 1 system restore point will be created now
> and
> this one is nicely removed afterwards.
>
> On my testboxes (XP and Vista) this makes sure that the tests run within the
> limits of the timeout (2 minutes). Before this patch the tests would take
> about
> 8 minutes on my Vista box (now 1.something).
>
> Changelog
>  Create a system restore point when needed

Please factor all of this out into functions.

+    /* Create a system restore point. We need to do this on a system
where 'System
+     * Restore' is active as we would otherwise timeout on these tests.
+     * By doing so we circumvent the multitude of restore points that
would have been
+     * created by all the installation and removal tests.
+     *
+     * The restore point is removed afterwards.
+     *
+     * On systems where 'System Restore' is not active, this won't
have any effect
+     * as the first call to SRSetRestorePointA will fail with a status of
+     * ERROR_SERVICE_DISABLED.
+     */

Does the comment really need to be that long?

-- 
James Hawkins

[msi/tests 3/3] Create a system restore point when needed

[Paul Vriens]

James Hawkins wrote:
> On Sat, Aug 30, 2008 at 3:14 PM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote:
> > Hi,
> >
> > The slowness on some machines for the install tests comes from the system
> > restore facility. When 'System Restore' is active you will find a great
> > number
> > of system restore points after these tests have run. Adding our own system
> > restore point circumvents this as Windows will now wait till we tell it that
> > the
> >  system change has ended. Only 1 system restore point will be created now
> > and
> > this one is nicely removed afterwards.
> >
> > On my testboxes (XP and Vista) this makes sure that the tests run within the
> > limits of the timeout (2 minutes). Before this patch the tests would take
> > about
> > 8 minutes on my Vista box (now 1.something).
> >
> > Changelog
> >  Create a system restore point when needed
>
> Please factor all of this out into functions.

I'll give that a shot.
> +    /* Create a system restore point. We need to do this on a system
> where 'System
> +     * Restore' is active as we would otherwise timeout on these tests.
> +     * By doing so we circumvent the multitude of restore points that
> would have been
> +     * created by all the installation and removal tests.
> +     *
> +     * The restore point is removed afterwards.
> +     *
> +     * On systems where 'System Restore' is not active, this won't
> have any effect
> +     * as the first call to SRSetRestorePointA will fail with a status of
> +     * ERROR_SERVICE_DISABLED.
> +     */
>
> Does the comment really need to be that long?

I think I can reduce that to a 2-liner or so. During testing I added stuff and 
stuff that's why it's maybe too much.

-- 
Cheers,

Paul.