Re: [PATCH 1/2] kernel32/tests: Make loader tests independent from each other

[Dmitry Timoshkov]

> +const static IMAGE_NT_HEADERS nt_header_template =

'static const' is a commonly accepted order.

There are various reasons of the current tests structure, if you are not
planning to add new tests please leave the code as it is please.

[PATCH 1/2] kernel32/tests: Make loader tests independent from each other

[Nikolay Sivov]

On 28.01.2015 18:20, Dmitry Timoshkov wrote:
> > +const static IMAGE_NT_HEADERS nt_header_template =
>
> 'static const' is a commonly accepted order.
>
> There are various reasons of the current tests structure, if you are not
> planning to add new tests please leave the code as it is please.

Are you going to name those reasons? Currently some tests depend on 
values set by previous tests.
That's clearly a broken behavior. test_import_resolution() does it right 
already by the way.

So why should I leave it as it is?

[PATCH 1/2] kernel32/tests: Make loader tests independent from each other

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > There are various reasons of the current tests structure, if you are not
> > planning to add new tests please leave the code as it is please.
>
> Are you going to name those reasons?

I don't think it's really important unless you are going to add new tests.
> Currently some tests depend on 
> values set by previous tests.
> That's clearly a broken behavior. test_import_resolution() does it right 
> already by the way.
>
> So why should I leave it as it is?

There is no actual reason to touch it, and these purely cosmetic changes
create conflicts with the tests I have in local tree.

[PATCH 1/2] kernel32/tests: Make loader tests independent from each other

[Nikolay Sivov]

On 28.01.2015 18:39, Dmitry Timoshkov wrote:
> Nikolay Sivov  wrote:
> > > There are various reasons of the current tests structure, if you are not
> > > planning to add new tests please leave the code as it is please.
> >
> > Are you going to name those reasons?
>
> I don't think it's really important unless you are going to add new tests.

That's how I saw they depend on each other, when I tried to add some more.
> > Currently some tests depend on
> > values set by previous tests.
> > That's clearly a broken behavior. test_import_resolution() does it right
> > already by the way.
> >
> > So why should I leave it as it is?
>
> There is no actual reason to touch it, and these purely cosmetic changes
> create conflicts with the tests I have in local tree.

Well, local merge problems can't be a reason to block cleanups like that.

[PATCH 1/2] kernel32/tests: Make loader tests independent from each other

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > So why should I leave it as it is?
> >
> > There is no actual reason to touch it, and these purely cosmetic changes
> > create conflicts with the tests I have in local tree.
>
> Well, local merge problems can't be a reason to block cleanups like that.

Since that's my code I'd prefer to keep it that way.