Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#533 - mock socket refactoring #539

Closed
wants to merge 4 commits into from
Closed

#533 - mock socket refactoring #539

wants to merge 4 commits into from

Conversation

exper0
Copy link
Contributor

@exper0 exper0 commented Jan 22, 2016

#533 - mock socket refactoring

@davvd
Copy link

davvd commented Jan 23, 2016

@exper0 Thanks, let me find someone who can review this pull request

@davvd
Copy link

davvd commented Jan 23, 2016

@longtimeago it's yours,please review

@@ -99,7 +99,15 @@
*/
@Test
public void handlesSocket() throws IOException {
final Socket socket = createMockSocket();
final Socket socket = createMockSocket(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@exper0 I didn't get what is the point of moving this code back from createMockSocket() method?
Do you understand issue you are trying to solve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@longtimeago absolutely no. I asked about it in #533 but decided it will be faster to create PR with anything. Could you please explain what should I do here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@exper0 from my point of view, the issue is pointless, unless you could get rid of mocking and introduce fake Stream as it was suggested by Yegor

@longtimeago
Copy link

@exper0 any updates here?

@exper0
Copy link
Contributor Author

exper0 commented Jan 31, 2016

@longtimeago not yet - I had to set 'away' mode a bit earlier. I will be able to regurn to the task next week

@exper0
Copy link
Contributor Author

exper0 commented Feb 4, 2016

@longtimeago please take a look at latest commit

@longtimeago
Copy link

@exper0 nice, thanks!

@longtimeago
Copy link

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Feb 4, 2016

@rultor merge

@longtimeago Thanks for your request. @yegor256 Please confirm this.

@longtimeago
Copy link

@yegor256 ping

@yegor256
Copy link
Owner

yegor256 commented Feb 9, 2016

@exper0 this is pointless... by mocking a JDK socket you're basically removing 90% of functionality from our classes. what is there left to test? just a few lines of code that makes calls to JDK sockets? they work, we can see that. but will they work with a real socket? we don't know, since you mock it out. we should use a real socket, not a fake one.

@longtimeago
Copy link

@yegor256 hum ... looks like we didn't get your previous comment #533 (comment)

@exper0 exper0 closed this Feb 19, 2016
@exper0 exper0 deleted the 533 branch February 19, 2016 22:25
@longtimeago
Copy link

@yegor256 This PR stuck is my tasks list. Could you please check?

@davvd
Copy link

davvd commented Feb 28, 2016

@elenavolokhova please, review this issue for QA compliance, as per par.24

@elenavolokhova
Copy link

@longtimeago
According to our quality rules:

The code reviewer found at least three problems in the code.

Only one issue was found during review.
Please confirm that you will try to find at least three problems next time.

@longtimeago
Copy link

@elenavolokhova I confirm

@elenavolokhova
Copy link

@davvd Quality is acceptable here.

@davvd
Copy link

davvd commented Feb 28, 2016

@davvd Quality is acceptable here.

@elenavolokhova thanks for the QA review, we'll work better next time

@davvd
Copy link

davvd commented Feb 28, 2016

@longtimeago added 10 mins to @elenavolokhova, for QA review, payment code is 78668553; thanks, paid, 24 mins to your account, payment ID is 78668566, task took 838 hours and 47 mins to complete; you're getting extra minutes here (c=9); added +24 to your rating, now it is equal to +6246

@yegor256
Copy link
Owner

@longtimeago everything is good now

@longtimeago
Copy link

@yegor256 yep, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants