- From: jgraham <web-platform-tests-notifications@w3.org>
- Date: Wed, 03 May 2017 20:45:40 GMT
- To: public-web-platform-tests-notifications@w3.org
Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.
---
*[tools/wptrunner/wptrunner/environment.py, line 99 at r1](https://reviewable.io:443/reviews/w3c/web-platform-tests/5771#-KjF2kAMwZ1MCr0_edvL:-KjF2kAMwZ1MCr0_edvM:bgga3pa) ([raw file](https://github.com/w3c/web-platform-tests/blob/cd58b81d82ac63d963c38f05827deb68b236cead/tools/wptrunner/wptrunner/environment.py#L99)):*
> ```Python
>
> def __enter__(self):
> for cm in self.env_extras:
> ```
I might put these last in the list, in case they depend on any other state. Clearly other state should not depend on these things.
---
*[tools/wptrunner/wptrunner/browsers/sauce.py, line 139 at r1](https://reviewable.io:443/reviews/w3c/web-platform-tests/5771#-KjF2XKggMVMy3R3A-d-:-KjF2XKhKrPY85l5-8PI:b-5kedhk) ([raw file](https://github.com/w3c/web-platform-tests/blob/cd58b81d82ac63d963c38f05827deb68b236cead/tools/wptrunner/wptrunner/browsers/sauce.py#L139)):*
> ```Python
> self.sauce_connect_binary = kwargs.get("sauce_connect_binary")
>
> if not self.sauce_connect_binary:
> ```
I think this shoud go in `__enter__` since it allocates resources.
---
*[tools/wptrunner/wptrunner/browsers/sauce.py, line 167 at r1](https://reviewable.io:443/reviews/w3c/web-platform-tests/5771#-KjF2dYWvZELhGVUm0D6:-KjF2dYWvZELhGVUm0D7:byqc3t4) ([raw file](https://github.com/w3c/web-platform-tests/blob/cd58b81d82ac63d963c38f05827deb68b236cead/tools/wptrunner/wptrunner/browsers/sauce.py#L167)):*
> ```Python
>
> def __exit__(self, *args):
> self.sc_process.terminate()
> ```
Also delete the tempdir, if any.
---
*Comments from [Reviewable](https://reviewable.io:443/reviews/w3c/web-platform-tests/5771)*
<!-- Sent from Reviewable.io -->
View on GitHub: https://github.com/w3c/web-platform-tests/pull/5771#issuecomment-299030074
Received on Wednesday, 3 May 2017 20:45:54 UTC