- 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