Skip to content
This repository was archived by the owner on Sep 21, 2021. It is now read-only.

Move starter proxy responsibilities into proxy set. #537

Merged
merged 38 commits into from
May 3, 2018
Merged

Conversation

diemol
Copy link
Contributor

@diemol diemol commented Apr 25, 2018

Comes from #527

Daniel Spasojevic and others added 23 commits April 16, 2018 14:24
Clean up formatting and remove unused imports.
Stop cleanup happening for idle outside of a test session
* Also make it possible to disable testing bot via a buildArg, this is
to allow builds on sites where testing bot is blocked due to
organisational Internet policies.
Request for Comment - Move starter proxy responsibilities into proxy set.
@pearj
Copy link
Collaborator

pearj commented Apr 26, 2018

/retest

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #537 into master will decrease coverage by 19.51%.
The diff coverage is 44.16%.

@@              Coverage Diff              @@
##             master     #537       +/-   ##
=============================================
- Coverage     79.83%   60.32%   -19.52%     
+ Complexity      582      472      -110     
=============================================
  Files            33       36        +3     
  Lines          2742     2999      +257     
  Branches        234      249       +15     
=============================================
- Hits           2189     1809      -380     
- Misses          409     1052      +643     
+ Partials        144      138        -6

@diemol
Copy link
Contributor Author

diemol commented May 1, 2018

OK, so now Travis is passing!
We need to check the unit tests and see what has to be enabled again.

Teardown re-registered proxies so that their threads are stopped gc'd.
@diemol
Copy link
Contributor Author

diemol commented May 1, 2018

@dspasojevic I think the email account you used for the commits is not associated to your GitHub account, so the commits are not being associated to your GitHub user.

@dspasojevic
Copy link
Contributor

Thanks for the heads up @diemol. I've linked my work accounts. It looks like the commits are correctly associated now.

@parberge
Copy link
Contributor

parberge commented May 2, 2018

@diemol Anything left to test or change now? Very excited about getting this merged and to get a new release of zalenium 😸

@diemol
Copy link
Contributor Author

diemol commented May 2, 2018

@peerster the thing is that some unit tests are failing now, and even if the integration tests are passing, I'd like to review the unit tests first :)
I'll try to check them during this week, but any help is appreciated. After that, I think we can be more confident to release.

@pearj
Copy link
Collaborator

pearj commented May 2, 2018

I just had a peek at why the test is failing, it's because the autostartproxy isn't expecting the proxy to register, so it doesn't accept the proxy registration

@dspasojevic
Copy link
Contributor

Thanks for looking @pearj. I added a SimpleRegistry that requires a bit less mocking for the other tests. I think that if I change that test to use it, it should pass. I'll give it a go.

@pearj
Copy link
Collaborator

pearj commented May 2, 2018

👍

@pearj
Copy link
Collaborator

pearj commented May 2, 2018

🚀

@dspasojevic
Copy link
Contributor

Hi @diemol, with @pearj's help, I've fixed the failing unit tests. Sorry - I should have checked those before pushing up.

The tests in DockerSeleniumStarterRemoteProxyTest are still commented out. Which of those tests do you think are valuable? If you let me know, I will look at porting them.

Do you think that it would be good to write some additional integration tests (e.g. test killing containers in minikube, etc) rather than writing more unit tests that involve quite elaborate mocks?

@diemol
Copy link
Contributor Author

diemol commented May 3, 2018

👍

1 similar comment
@pearj
Copy link
Collaborator

pearj commented May 3, 2018

👍

@diemol diemol merged commit 9abf3a2 into master May 3, 2018
@diemol diemol deleted the merge-527 branch May 3, 2018 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants