Use context manager to manage OS environment in tests

Use context manager to manage OS environment in tests

Currently tests use manual context management (try: ... finally: ...)
This is cleaner and more robust with a context manager:
https://docs.python.org/2/library/contextlib.html

Also fixes some bugs and typos.

Tests are:
test/cflags/gyptest-cflags.py (make + ninja)
test/cxxflags/gyptest-cxxflags.py (ninja)

R=thakis
TEST=PYTHONPATH=$CHROMIUM_DIR/tools/gyp/test/lib TESTGYP_FORMAT=make test/cflags/gyptest-cflags.py 
TEST=PYTHONPATH=$CHROMIUM_DIR/tools/gyp/test/lib TESTGYP_FORMAT=ninja test/cflags/gyptest-cflags.py 
TEST=PYTHONPATH=$CHROMIUM_DIR/tools/gyp/test/lib TESTGYP_FORMAT=ninja test/cxxflags/gyptest-cxxflags.py

r1893

[Nils Barth (inactive), 2014-04-08 08:56:32]

Hi Nico,
When looking up how GYP handles compiler options, I noticed this cleanup
possibility.
WDYT?

https://codereview.chromium.org/228323002/diff/1/test/cxxflags/gyptest-cxxfla...
File test/cxxflags/gyptest-cxxflags.py (left):

https://codereview.chromium.org/228323002/diff/1/test/cxxflags/gyptest-cxxfla...
test/cxxflags/gyptest-cxxflags.py:23: os.eniron=env_stack.pop()
Ow, notice the typos?
os.eniron (not os.en*v*iron)
Also shouldn't set os.environ: should clear()/update()

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py#newcode50
test/lib/TestGyp.py:50: @contextmanager
This is a simple single-use context manager (CM), which is fine:
these nest correctly.
We only need a re-entrant CM if reusing the same CM multiple times, but we want
to set the local environment (formally, new frame) each time.

https://docs.python.org/3/library/contextlib.html#single-use-reusable-and-ree...

[Nico, 2014-04-08 15:35:12]

lgtm!

(I think this code was written before contextmanagers existed in python, or at
least before we could use them.)

[Stefan Haller, 2014-04-08 17:05:40]

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py#newcode55
test/lib/TestGyp.py:55: yield
Sorry for jumping in here: for correctness, this should read

try:
  yield
finally:
  os.environ.clear()
  os.environ.update(env_copy)

Otherwise the environment won't be restored for failing tests, possibly
affecting the results of other tests that run after it.

[Nils Barth (inactive), 2014-04-09 03:09:18]

Thanks Stefan!

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/228323002/diff/1/test/lib/TestGyp.py#newcode55
test/lib/TestGyp.py:55: yield
On 2014/04/08 17:05:41, Stefan Haller wrote:
> Sorry for jumping in here: for correctness, this should read
> 
> try:
>   yield
> finally:
>   os.environ.clear()
>   os.environ.update(env_copy)
> 
> Otherwise the environment won't be restored for failing tests, possibly
> affecting the results of other tests that run after it.

Oh, good catch -- thanks!
*don't* need try: ... finally: ... for manually written context managers
with cleanup in __exit__,
but *do* need try: ... finally: ... for @contextlib.contextmanager

[Nils Barth (inactive), 2014-04-09 05:58:24]

One note:
the cxxflags test *failed* (for make) once I corrected it to
actually reset the environment.
This can be seen more simply by correcting
|os.eniron = ...| to |os.environ = ...|

This is because make apparently needs the same environment
when building as during gyp?
However, this does work for ninja.
In any case, this was an existing difference between the tests
(same env vs. diff env), so I've documented it in the module
docstring.

If I've messed up anything, please correct!

[Nils Barth (inactive), 2014-04-09 05:58:43]

The CQ bit was checked by nbarth@chromium.org

[Nils Barth (inactive), 2014-04-09 05:58:46]

The CQ bit was unchecked by nbarth@chromium.org

[Nils Barth (inactive), 2014-04-09 06:00:19]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-04-09 06:05:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 06:05:17]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Nils Barth (inactive), 2014-04-09 06:08:23]

Actually Nico, looks like CQ doesn't like this;
could you commit yourself, unless I should dcommit?

[Nico, 2014-04-09 17:50:06]

On 2014/04/09 05:58:24, Nils Barth wrote:
> One note:
> the cxxflags test *failed* (for make) once I corrected it to
> actually reset the environment.
> This can be seen more simply by correcting
> |os.eniron = ...| to |os.environ = ...|
> 
> This is because make apparently needs the same environment
> when building as during gyp?
> However, this does work for ninja.

Yes, CC is read at gyp time by the ninja generator, but it's a build-time thing
with make I think.

> In any case, this was an existing difference between the tests
> (same env vs. diff env), so I've documented it in the module
> docstring.
> 
> If I've messed up anything, please correct!

[Nico, 2014-04-09 17:50:28]

On 2014/04/09 06:08:23, Nils Barth wrote:
> Actually Nico, looks like CQ doesn't like this;
> could you commit yourself, unless I should dcommit?

The cq doesn't work for gyp, hence me asking if I should land this for you :-)

[Nils Barth (inactive), 2014-04-10 00:44:31]

On 2014/04/09 17:50:28, Nico wrote:
> The cq doesn't work for gyp, hence me asking if I should land this for you :-)

Got it. Yes please, could you land it for me?

[Nils Barth (inactive), 2014-04-10 00:46:20]

On 2014/04/09 17:50:06, Nico wrote:
> > This is because make apparently needs the same environment
> > when building as during gyp?
> > However, this does work for ninja.
> 
> Yes, CC is read at gyp time by the ninja generator, but it's a build-time
thing
> with make I think.

Conveniently, we now have a (working) test for this (for ninja);
I take it we don't need an explicit test for make giving different environments
at gyp time and build time?

[Nico, 2014-04-10 21:47:32]

r1893

[scottmg, 2014-04-10 23:22:30]

On 2014/04/10 21:47:32, Nico wrote:
> r1893

It looks like this broke test\cxxflags\gyptest-cxxflags.py on windows.

[Nils Barth (inactive), 2014-04-11 02:22:59]

On 2014/04/10 21:47:32, Nico wrote:
> r1893

Thanks!

[Nils Barth (inactive), 2014-04-11 02:28:01]

On 2014/04/10 23:22:30, scottmg wrote:
> On 2014/04/10 21:47:32, Nico wrote:
> > r1893
> 
> It looks like this broke test\cxxflags\gyptest-cxxflags.py on windows.

? Any details ?
(I don't have a Windows machine, so I can't debug.)
This can be fixed by moving the |test.build| command *into* the local context,
but this is revealing a difference between ninja-Linux and ninja-Windows AFAICT,
and perhaps needs GYP changes to fix this?


The main meaningful difference in test\cxxflags\gyptest-cxxflags.py
is that it's now testing ninja, not make.
"""In this test, gyp happens within a local environment, but build outside of
it."""

The key code is as below, with the key point being that the build happens
*outside*
the local environment (unlike in the other test).
This breaks make-Linux, but works in ninja-Linux, hence why I updated the test.
Maybe it doesn't work on ninja-Windows?

Code:
with TestGyp.LocalEnv({'CXXFLAGS': '-O0'}):
  test.run_gyp('cxxflags.gyp')

test.build('cxxflags.gyp')

Could fix as:
with TestGyp.LocalEnv({'CXXFLAGS': '-O0'}):
  test.run_gyp('cxxflags.gyp')
  test.build('cxxflags.gyp')

...but that changes the test meaningfully.

The comment is:
# We reset the environ after calling gyp. When the auto-regeneration happens,
# the same define should be reused anyway.

Hope this helps!

[Nils Barth (inactive), 2014-04-11 02:30:08]

Concretely, would you like to:
1. Apply a CL moving build into the environment (should fix test)
2. Revert this CL
3. Investigate Linux/Windows behavior difference yourselves?

#1 is probably easiest, but maybe you want to look into this more?

[Nils Barth (inactive), 2014-04-11 02:43:12]

On 2014/04/11 02:30:08, Nils Barth wrote:
> Concretely, would you like to:
> 1. Apply a CL moving build into the environment (should fix test)
> 2. Revert this CL
> 3. Investigate Linux/Windows behavior difference yourselves?
> 
> #1 is probably easiest, but maybe you want to look into this more?

I've posted a CL doing #1 (modify test so should pass on ninja-Windows):
Change cxxflags test to build in same env as gyp (+ test make and ninja)
https://codereview.chromium.org/233713003/

...in case this is the solution we want.