#865 Added RqFromTest and HttpServletRequestFake #941

fabriciofx · 2019-01-12T07:28:51Z

As per #865. Created:

RqFromTest
HttpServletRequestFake and ServletInputStreamOf to support it
HttpServletResquetFakTest

0crat · 2019-01-12T07:30:44Z

Job #941 is now in scope, role is REV

0crat · 2019-01-12T07:31:44Z

This pull request #941 is assigned to @marceloamadeu /z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso /z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

codecov-io · 2019-01-12T07:35:30Z

Codecov Report

Merging #941 into master will decrease coverage by 1.81%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #941      +/-   ##
============================================
- Coverage      74.1%   72.29%   -1.82%     
  Complexity      971      971              
============================================
  Files           222      224       +2     
  Lines          4785     4905     +120     
  Branches        361      364       +3     
============================================
  Hits           3546     3546              
- Misses         1089     1209     +120     
  Partials        150      150

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/takes/servlet/SrvTake.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...n/java/org/takes/servlet/ServletInputStreamOf.java	`0% <0%> (ø)`	`0 <0> (?)`
src/main/java/org/takes/servlet/RqFrom.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...java/org/takes/servlet/HttpServletRequestFake.java	`0% <0%> (ø)`	`0 <0> (?)`

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 d5f747d...afd95b0. Read the comment docs.

fabriciofx · 2019-01-16T03:53:13Z

@marceloamadeu ping

fabriciofx · 2019-01-18T13:29:20Z

@marceloamadeu can I help you with anything?

fabriciofx · 2019-01-20T04:46:42Z

@marceloamadeu ping

fabriciofx · 2019-01-21T21:52:21Z

@marceloamadeu ping

marceloamadeu

@fabriciofx Please take a look at the comment.

marceloamadeu · 2019-01-22T13:34:22Z

src/test/java/org/takes/servlet/RqFromTest.java

+                    )
+                )
+            ).printBody(),
+            Matchers.containsString(content)


@fabriciofx Why use static matcher? I suggest using org.hamcrest.core.StringContains

marceloamadeu · 2019-01-22T13:38:51Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+        MatcherAssert.assertThat(
+            "Can't get the request method",
+            new HttpServletRequestFake(new RqFake()).getMethod(),
+            Matchers.equalTo("GET")


@fabriciofx I also suggest using new IsEqual <> ("GET") instead of
Matchers.equalTo ("GET")

marceloamadeu · 2019-01-22T14:10:08Z

src/test/java/org/takes/servlet/RqFromTest.java

+                    )
+                )
+            ).printHead(),
+            Matchers.startsWith(


@fabriciofx It is possible to replace org.hamcrest.core.StringStartsWith?

marceloamadeu · 2019-01-22T14:22:12Z

src/test/java/org/takes/servlet/RqFromTest.java

+                )
+            ).printHead(),
+            Matchers.startsWith(
+                Joiner.on("\r\n").join(


@fabriciofx Why not use org.cactoos.text.JoinedText? (Example: https://github.com/yegor256/takes/blob/master/src/test/java/org/takes/tk/TkHtmlTest.java)

fabriciofx · 2019-01-23T00:43:36Z

@marceloamadeu Please, can you check out again?

marceloamadeu · 2019-01-23T15:49:54Z

@fabriciofx Tks!!!

marceloamadeu · 2019-01-23T15:50:37Z

@rultor merge

rultor · 2019-01-23T15:55:31Z

@rultor merge

@marceloamadeu Thanks for your request. @paulodamaso Please confirm this.

paulodamaso

@fabriciofx Thank for the PR, take a look at the comments please

paulodamaso · 2019-01-23T17:24:43Z

src/main/java/org/takes/servlet/RqFrom.java

-            final String header = names.nextElement();
-            head.add(
+        head.add(new HttpHead(this.sreq).toString());
+        final Collection<String> names = Collections.list(


@fabriciofx Let's use something from cactoos here, I think that ListOf should do the trick.

@paulodamaso Unfortunately, ListOf doesn't work with Enumeration therefore I must use Collections.list. Ah, I've opened a issue into cactoos project to create a new constructor to ListOf to support Enumeration but it was denied (because is too old).

@fabriciofx I've seen the issue in cactoos... Well, as they decided to not support Enumeration we should let it as is then, thanks.

paulodamaso · 2019-01-23T17:27:41Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+                    )
+                ).getHeaders("testheader")
+            ),
+            Matchers.hasItems("someValue")


@fabriciofx Avoid using static matchers, use IsCollectionContaining or something similar here.

@paulodamaso IsCollectionContaining usage is something like: IsCollectionContaining.hasItems("bla"). I think just the same, but if you want, I can change it.

@fabriciofx No, I'm talking about new IsIteratorContaining(Macther) form, avoiding the static method usage.

paulodamaso · 2019-01-23T17:28:07Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+    public void headerNames() {
+        MatcherAssert.assertThat(
+            "Can't get the header names",
+            Collections.list(


@fabriciofx Same comment as above

@paulodamaso Same above too. The method returns an Enumeration and ListOf doesn't support it.

paulodamaso · 2019-01-23T17:28:30Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+                    )
+                ).getHeaderNames()
+            ),
+            Matchers.hasItems("host", "anyheader", "crazyheader")


@fabriciofx Same comment about IsCollectionContaining

@fabriciofx Same above.

paulodamaso · 2019-01-23T17:30:06Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+        MatcherAssert.assertThat(
+            "Can't get the request method",
+            new HttpServletRequestFake(new RqFake()).getMethod(),
+            new IsEqual<>("GET")


@fabriciofx Please use RqMethod.GET instead of "GET" here

@paulodamaso Done.

paulodamaso · 2019-01-23T17:32:23Z

src/test/java/org/takes/servlet/RqFromTest.java

+ * @since 1.15
+ * @checkstyle ClassDataAbstractionCouplingCheck (500 lines)
+ */
+@SuppressWarnings("PMD.AvoidDuplicateLiterals")


@fabriciofx I think that you could create constants for the repeated values in this test and avoid using this supression WDYT?

@paulodamaso Can I use the same constant to all tests? It will works only in this case...

@fabriciofx Or you could use final variables inside your test methods, for example, 'final String get = "GET /a-test"' and final String foo = "foo: bar" for header() and use this variables in the test. You don't need to use constants because values are not shared between the tests.

paulodamaso

@fabriciofx Take a look at the comments, please. Also, I've noticed that the test methods signatures don't follow what explained in https://www.yegor256.com/2014/04/27/typical-mistakes-in-java-code.html#test-method-names , please fix them

paulodamaso · 2019-01-23T23:50:34Z

src/test/java/org/takes/servlet/HttpServletRequestFakeTest.java

+                    )
+                ).getHeaders("testheader")
+            ),
+            Matchers.hasItems("someValue")


@fabriciofx No, I'm talking about new IsIteratorContaining(Macther) form, avoiding the static method usage.

paulodamaso · 2019-01-23T23:55:52Z

src/test/java/org/takes/servlet/RqFromTest.java

+ * @since 1.15
+ * @checkstyle ClassDataAbstractionCouplingCheck (500 lines)
+ */
+@SuppressWarnings("PMD.AvoidDuplicateLiterals")


@fabriciofx Or you could use final variables inside your test methods, for example, 'final String get = "GET /a-test"' and final String foo = "foo: bar" for header() and use this variables in the test. You don't need to use constants because values are not shared between the tests.

fabriciofx · 2019-01-24T03:53:29Z

@paulodamaso Done. Please, check it again. And, if it's ok, merge it!

paulodamaso

@fabriciofx Just one more comment, please

paulodamaso · 2019-01-24T16:03:26Z

src/main/java/org/takes/servlet/ServletInputStreamOf.java

+import javax.servlet.ServletInputStream;
+
+/**
+ * ServletInputStreamOf.


@fabriciofx Please enrich this javadoc so we can know what this class is about.

fabriciofx · 2019-01-24T16:18:30Z

@paulodamaso Done. Please, check out.

paulodamaso · 2019-01-24T16:28:52Z

@fabriciofx Thanks!

paulodamaso · 2019-01-24T16:29:04Z

@rultor merge

rultor · 2019-01-24T16:29:38Z

@rultor merge

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

rultor · 2019-01-24T16:49:56Z

@rultor merge

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

0crat · 2019-01-24T17:33:58Z

@ypshenychka /z please review this job completed by @marceloamadeu /z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed

0crat · 2019-01-24T17:34:16Z

Code review was too long (12 days), architects (@paulodamaso) were penalized, see §55

0crat · 2019-01-24T17:38:38Z

The job #941 is now out of scope

0crat · 2019-01-24T17:44:39Z

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @paulodamaso /z

ypshenychka · 2019-01-24T20:38:10Z

@marceloamadeu According to our QA Rules:

Messages in a job always start with a name of a user they are addressed to.

Please correct your code review comments by indicating an addressee in the beginning.

marceloamadeu · 2019-01-25T10:44:52Z

@ypshenychka Adjusted.

marceloamadeu · 2019-01-25T19:23:38Z

@ypshenychka ping

ypshenychka · 2019-01-26T09:35:02Z

@marceloamadeu thanks

ypshenychka · 2019-01-26T09:35:15Z

@0crat quality acceptable

0crat · 2019-01-26T10:20:00Z

@0crat quality acceptable (here)

@ypshenychka The project doesn't have enough funds, can't make a payment

0crat · 2019-01-26T10:21:24Z

Order was finished, quality is "acceptable": +15 point(s) just awarded to @marceloamadeu /z

0crat · 2019-01-26T10:21:27Z

Quality review completed: +8 point(s) just awarded to @ypshenychka /z

0crat added the scope label Jan 12, 2019

marceloamadeu suggested changes Jan 22, 2019

View reviewed changes

fabriciofx added 2 commits January 22, 2019 19:35

yegor256#865 Added RqFromTest and HttpServletRequestFake

ec5d5d5

yegor256#865 Fix tests as requested by reviewer

d847792

fabriciofx force-pushed the 865 branch from bade628 to d847792 Compare January 23, 2019 00:42

paulodamaso suggested changes Jan 23, 2019

View reviewed changes

yegor256#865 More fixes as requested by reviewer

ad2d0c7

paulodamaso suggested changes Jan 24, 2019

View reviewed changes

yegor256#865 Javadoc improved

afd95b0

paulodamaso approved these changes Jan 24, 2019

View reviewed changes

rultor merged commit afd95b0 into yegor256:master Jan 24, 2019

0crat removed the scope label Jan 24, 2019

#865 Added RqFromTest and HttpServletRequestFake #941

#865 Added RqFromTest and HttpServletRequestFake #941

Conversation

fabriciofx commented Jan 12, 2019

0crat commented Jan 12, 2019

0crat commented Jan 12, 2019

codecov-io commented Jan 12, 2019 • edited

Codecov Report

fabriciofx commented Jan 16, 2019

fabriciofx commented Jan 18, 2019 • edited

fabriciofx commented Jan 20, 2019

fabriciofx commented Jan 21, 2019

marceloamadeu left a comment

Choose a reason for hiding this comment

marceloamadeu Jan 22, 2019 • edited

Choose a reason for hiding this comment

marceloamadeu Jan 22, 2019 • edited

Choose a reason for hiding this comment

marceloamadeu Jan 22, 2019 • edited

Choose a reason for hiding this comment

marceloamadeu Jan 22, 2019 • edited

Choose a reason for hiding this comment

fabriciofx commented Jan 23, 2019

marceloamadeu commented Jan 23, 2019

marceloamadeu commented Jan 23, 2019

rultor commented Jan 23, 2019

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriciofx commented Jan 24, 2019 • edited

paulodamaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriciofx commented Jan 24, 2019

paulodamaso commented Jan 24, 2019

paulodamaso commented Jan 24, 2019

rultor commented Jan 24, 2019

rultor commented Jan 24, 2019

0crat commented Jan 24, 2019

0crat commented Jan 24, 2019

0crat commented Jan 24, 2019

0crat commented Jan 24, 2019

ypshenychka commented Jan 24, 2019

marceloamadeu commented Jan 25, 2019

marceloamadeu commented Jan 25, 2019

ypshenychka commented Jan 26, 2019

ypshenychka commented Jan 26, 2019

0crat commented Jan 26, 2019

0crat commented Jan 26, 2019

0crat commented Jan 26, 2019

codecov-io commented Jan 12, 2019 •

edited

fabriciofx commented Jan 18, 2019 •

edited

marceloamadeu Jan 22, 2019 •

edited

marceloamadeu Jan 22, 2019 •

edited

marceloamadeu Jan 22, 2019 •

edited

marceloamadeu Jan 22, 2019 •

edited

fabriciofx commented Jan 24, 2019 •

edited