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

#876 corrected test for PsFacebook #885

Merged
merged 2 commits into from
Dec 12, 2018
Merged

#876 corrected test for PsFacebook #885

merged 2 commits into from
Dec 12, 2018

Conversation

krzyk
Copy link
Contributor

@krzyk krzyk commented Dec 7, 2018

#876

  • corrected test for PsFacebook to generate correct unicode characters (and not control sequences - random() was generating those and that's why the test was sometimes failing)

@0crat
Copy link
Collaborator

0crat commented Dec 7, 2018

Job #885 is now in scope, role is REV

@krzyk
Copy link
Contributor Author

krzyk commented Dec 11, 2018

@paulodamaso no reviewer was assigned here

Copy link
Contributor

@paulodamaso paulodamaso left a comment

Choose a reason for hiding this comment

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

@krzyk Please take a look at the comment

@@ -52,15 +53,20 @@
@Test
public final void canLogin() throws Exception {
final String identifier = RandomStringUtils.randomAlphanumeric(10);
final RandomStringGenerator generator =
Copy link
Contributor

Choose a reason for hiding this comment

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

@krzyk I think that we could replace RandomStringGenerator and RandomStringUtils usage with RandomText from cactoos, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulodamaso But RandomText configuration is cumbersome, I would need to list all UTF characters there and do filtering, all this is done automatically byt RandomStringGenerator and RandomStringUtils.

Copy link
Contributor

Choose a reason for hiding this comment

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

@krzyk RandomText comes with an pre-defined set of characters, so you'd just need to filter the unwanted ones, I think that it'll be enough for our test cases, WDYT?

Copy link
Contributor Author

@krzyk krzyk Dec 11, 2018

Choose a reason for hiding this comment

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

@paulodamaso OK, and are you OK with IntStream for generating the list of characters (because I need characters that are not ASCII)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@krzyk Yes, there is something that should I be worried about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulodamaso No, I was worried that I would need to embed zillions of classes from cactoos (because there is no direct way to generate a List or Iterator with provided seed and function) to get the same thing I can do in 4-5 lines with streams :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulodamaso So after reviewing it more, it is impossible to do. RandomText should accept a list of codepoints, but it accepts only Characters. One codepoint can result in mutliple chars (and the order is important here). And here we want to test unicode chars that can be outside of the simple 64k space that can be represented by a single char.

Copy link
Contributor

Choose a reason for hiding this comment

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

@krzyk Okay, so that's nothing else to do here, thanks!

@paulodamaso
Copy link
Contributor

@krzyk Please just see why your build is failing on travis

@krzyk
Copy link
Contributor Author

krzyk commented Dec 12, 2018

@paulodamaso done

@codecov-io
Copy link

codecov-io commented Dec 12, 2018

Codecov Report

Merging #885 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #885      +/-   ##
============================================
+ Coverage     75.45%   75.48%   +0.03%     
- Complexity      977      981       +4     
============================================
  Files           220      220              
  Lines          4717     4724       +7     
  Branches        368      369       +1     
============================================
+ Hits           3559     3566       +7     
  Misses         1003     1003              
  Partials        155      155
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/takes/rs/Body.java 93.47% <0%> (ø) 0% <0%> (ø) ⬇️
src/main/java/org/takes/misc/Utf8String.java 100% <0%> (ø) 4% <0%> (ø) ⬇️
src/main/java/org/takes/rs/RsPrint.java 80% <0%> (+0.4%) 14% <0%> (+1%) ⬆️
src/main/java/org/takes/rs/RsVelocity.java 86.11% <0%> (+2.77%) 10% <0%> (+3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a11256...2bf5e17. Read the comment docs.

@paulodamaso
Copy link
Contributor

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Dec 12, 2018

@rultor merge

@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 2bf5e17 into yegor256:master Dec 12, 2018
@rultor
Copy link
Collaborator

rultor commented Dec 12, 2018

@rultor merge

@paulodamaso Done! FYI, the full log is here (took me 23min)

@0crat 0crat removed the scope label Dec 12, 2018
@0crat
Copy link
Collaborator

0crat commented Dec 12, 2018

Job gh:yegor256/takes#885 is not assigned, can't get performer

@0crat
Copy link
Collaborator

0crat commented Dec 12, 2018

The job #885 is now out of scope

@krzyk krzyk deleted the 876 branch December 12, 2018 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants