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

added tests for VerboseIterable and VerboseIterator #125

Closed
wants to merge 3 commits into from

Conversation

celezar
Copy link

@celezar celezar commented Apr 4, 2015

I have created testst for VerboseIterable and VerboseIterator as requested in issue #111.

@davvd
Copy link

davvd commented Apr 4, 2015

Many thanks for the PR, let me find a reviewer for it

@davvd
Copy link

davvd commented Apr 4, 2015

@ggajos review this one,please

@ggajos
Copy link

ggajos commented Apr 7, 2015

@celezar please fix qulice issues

public ExpectedException expectedEx = ExpectedException.none();

@Test
public void testIterator() throws Exception {
Copy link

Choose a reason for hiding this comment

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

@celezar
Copy link
Author

celezar commented Apr 7, 2015

@ggajos I have made the changes so there are no more qulice issues. I had some problem with running qulice test from Idea but was able to do so from CLI. I disabled some rules. VisibilityModifierCheck was necessary to disable on field expected.

private static String two = "2";
private static String three = "3";
private static String errormessage = "Error message";

Copy link

Choose a reason for hiding this comment

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

@celezar let's avoid extracting fields when used only once

Copy link
Author

Choose a reason for hiding this comment

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

they are used at least two times. i extracted them because qulice was complaining.

Copy link

Choose a reason for hiding this comment

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

@celezar you can try to use different string, let's not couple tests together

@ggajos
Copy link

ggajos commented Apr 8, 2015

@celezar all good, just few comments

@ggajos
Copy link

ggajos commented Apr 21, 2015

@celezar any update here?

@celezar
Copy link
Author

celezar commented Apr 21, 2015

@ggajos i have asked for reassignment of this task

@ggajos
Copy link

ggajos commented Apr 21, 2015

@celezar ok, so we can close this PR without merge right?

@celezar
Copy link
Author

celezar commented Apr 21, 2015

@ggajos i guess so but not sure what the procedure is. i have requested reassignment but it is still not assigned to someone else.

@yegor256
Copy link
Owner

@celezar just close the pull request and that's it

@celezar celezar closed this Apr 23, 2015
@davvd
Copy link

davvd commented Apr 24, 2015

@elenavolokhova please, review this task for compliance with our quality rules

@elenavolokhova
Copy link

@davvd Despite this PR was not merged, reviewer did his work well and should be rewarded. Still it can't be marked as perfect.
Quality is acceptable here.

@elenavolokhova
Copy link

@davvd Quality is acceptable.

@davvd
Copy link

davvd commented May 6, 2015

@davvd Quality is acceptable.

@elenavolokhova thanks a lot, next time everybody should try to make it better

@davvd
Copy link

davvd commented May 9, 2015

@ggajos I added 10 mins to @elenavolokhova (for QA review) in transaction 56843371... 19 mins sent to your balance (ID AP-1HW39538JE671424W), many thanks! It took 451 hours and 36 mins.... you're getting extra minutes here (c=4)... added +19 to your rating, now it is equal to +3071

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

Successfully merging this pull request may close these issues.

None yet

6 participants