Add tests for ConsoleReporter. #5675

piyush-jaiswal · 2017-02-04T22:52:14Z

This commit adds tests for the ConsoleReporter which includes tests for the feature added by #5667.

piyush-jaiswal · 2017-02-04T23:00:32Z

Added 2 files, am not certain about the coding style.

som-snytt · 2017-02-05T01:35:37Z

I think it's the git style. You didn't intend to commit all the extra files.

piyush-jaiswal · 2017-02-05T07:12:17Z

Yeah, exactly. Anyways, just to clarify, I have signed the CLA.

piyush-jaiswal · 2017-02-05T07:14:02Z

@adriaanm, could you review it?

som-snytt · 2017-02-05T17:51:55Z

Since ConsoleReporter takes a reader and writer, the unit test should supply them and not use Console.

(Using Console in particular reduces test isolation, a problem if you want to run many tests without forking.)

This test could also subsume the new test at #5667 where probably an end-to-end test is overkill.

Note that maxerr isn't fixed at 100 now, so setting it explicitly for the test is useful (instead of testing the default).

som-snytt · 2017-02-05T17:54:56Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+
+      // finish test
+      reporter.ERROR.count = 0
+      reporter finish;    // don't know why but I'm requiring ';' here, otherwise the file does not compile


I'd suggest not enabling postfixOps at all.

som-snytt · 2017-02-05T17:58:42Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+      val reporter = new ConsoleReporter(new Settings())
+
+      // printMessage test
+      reporter printMessage("Hello World!")


I'd suggest using dot-selection, especially with parens.

reporter.printMessage("hello, world.") reporter printMessage "hello, world." // Recently, I use this style less, but community hasn't outlawed it.

som-snytt · 2017-02-05T18:40:03Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+      err_out.reset
+
+      reporter.WARNING.count = 3
+      reporter.ERROR.count = 10


Probably when the reporter API is nailed down some more, this should be disallowed. Maybe Reporter should have protected increment(severity).

piyush-jaiswal · 2017-02-05T19:54:44Z

Pardon me @som-snytt , I didn't know you were my assigned reviewer. I will make the changes as suggested but in the meanwhile, my midterms are around the corner. Is it okay if this can wait for some days?

som-snytt · 2017-02-05T19:59:00Z

Absolutely, reviewing and updating are on a volunteer basis!

piyush-jaiswal · 2017-02-11T21:32:17Z

Hey @som-snytt, I made the changes. Could you review and suggest further changes if needed.

som-snytt · 2017-02-12T09:16:24Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+  @Test
+  def unitTest: Unit = {
+    val noPos = NoPosition
+    val path = "test/junit/scala/tools/nsc/reporters/file_ConsoleReporterTest.txt"


I'm not clear on why a file is needed. Source can be just a byte array.

som-snytt · 2017-02-12T09:17:15Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    // printMessage test
+    reporter.printMessage("Hello World!")
+    assertEquals("Hello World!\n", err_out.toString)
+    err_out.reset


Could refactor this pattern to a method.

I'm not sure that I get you correctly. Do you want me to group the related tests inside a method, for example the tests for printMessage inside a single method?

piyush-jaiswal · 2017-02-13T17:36:53Z

Made the changes. Still not sure, if it's exactly what you wanted.

piyush-jaiswal · 2017-02-14T14:48:01Z

Hey @som-snytt, could you review the changes?

som-snytt

I made a couple of small comments. I'm trying out the "review" button for the first time, which really complicates the workflow. (note to github)

You need to rebase and clean up the git history, which includes spurious merge commits. They want to see your one squashed commit before they merge. (as a rule)

som-snytt · 2017-02-14T15:08:02Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    val input = new StringReader("s\n")
+    val input_reader = new BufferedReader(input)
+    val setting = new Settings()
+    setting.maxerrs.tryToSet(List("50"))  //changing maxerrs


settings.maxerrs.value = 50

som-snytt · 2017-02-14T15:12:21Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    def printMessageTest(): Unit = {
+      reporter.printMessage("Hello World!")
+      assertEquals("Hello World!\n", err_out.toString)
+      err_out.reset


You could capture this repeated pattern in a routine that performs the assert and the reset. I often see simple helpers like that in unit tests.

som-snytt · 2017-02-14T15:16:25Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+
+      // printColumnMarkerTest test with DefinedPosition
+      reporter.printColumnMarker(posWithSource)
+      assertEquals("    ^\n", err_out.toString)


This assumes the platform line ending, perhaps. text.lines converts to lines.

som-snytt · 2017-02-14T15:27:09Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    @Test
+    def displayPromptTest(): Unit = {
+      val stack = new ByteArrayOutputStream
+      System.setErr(new PrintStream(stack))  // used for checking the stack trace of displayPrompt()


Console.withErr. Again, it's easier to encapsulate with a method that does the setup. Allocating a BAOS for every test is easier than stack.reset. As a style matter, maybe the idea is that it's worth reusing a heavyweight object?

som-snytt · 2017-02-14T15:28:30Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+        val input = new StringReader("r\n")
+        val input_reader = new BufferedReader(input)
+        val reporter = new ConsoleReporter(new Settings(), input_reader, output_writer)
+        reporter.displayPrompt


Not a huge deal, but I'd expect parens to demonstrate a side-effecting application. That's the standard advice. This looks like it just accesses a value.

piyush-jaiswal · 2017-02-18T18:33:57Z

/rebuild

som-snytt · 2017-02-18T19:12:56Z

You have to fix the git history. Your PR should show your one commit.

piyush-jaiswal · 2017-02-19T07:30:08Z

I didn't push form the '2.12.x' branch after rebasing it, think that was the root of the problem. Thanks @som-snytt !

I added the protected increment and reset severity functions in ConsoleReporter and also modified the tests. Hope you can take a look at it.

adriaanm · 2017-02-20T22:48:45Z

@piyush-jaiswal, note that you seem to have accidentally committed a bunch of unrelated files. Please also review our guidelines on how to format a good commit message. As it stands, your commit title is too long, is not in the present tense, and there's no elaboration in the commit message itself (for example: what does this do? why? what was wrong before?)

Thanks for reviewing @som-snytt! When you feel this PR is ready, feel free to merge.

piyush-jaiswal · 2017-02-22T04:31:26Z

Made the changes. Kindly point out other issues if present.

som-snytt · 2017-02-28T22:00:45Z

Sorry, just a busy time, albeit not midterms. If I suggested adding API to ConsoleReporter just for a test, don't listen to that guy. Also, there was just a commit to let it take two writers, for error and echo. That might affect your test.

You can actually fix something by changing it to use printStackTrace(writer). It becomes testable and correct. I guess you can't test the abort option.

piyush-jaiswal · 2017-03-01T15:51:56Z

Okay, I've changed my tests according to the commit and yeah I don't think it's possible to test the abort option too.

piyush-jaiswal · 2017-03-02T23:09:12Z

review @som-snytt ?

piyush-jaiswal · 2017-03-08T10:23:49Z

Is there any further issue present? I am finding it difficult to spotting them on my own.

som-snytt · 2017-03-08T16:52:16Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+      if(echoWriterOut == null)
+        new ConsoleReporter(new Settings(), reader, writer) 
+      else
+      {


Repo style is space after if if ( and open brace on same line else {.

som-snytt · 2017-03-08T16:52:58Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+
+
+  def testHelper(checkBlank: Boolean, hasPos: Boolean, message: String, severity: String): Unit = {
+    if(checkBlank) assertEquals("", writerOut.toString)


the flag is redundant with message = "". Possibly message.isEmpty && severity.isEmpty.

som-snytt · 2017-03-08T17:03:04Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+      }
+    }
+    writerOut.reset
+  }


The test rig could be: testHelper(pos: Position = NoPosition, msg: String, severity: String = "")(test: Position => Unit).

testHelper(msg = "hello, world")(_ => reporter.printMessage("hello, world")).

testHelper(myPos, "test")(reporter.print(_, "test", INFO)) or similar.

The hasPos flag is pos.isDefined.

I lose track of the true, false pairs in the tests.

I'm not sure if we require Position as an argument of the function stored in the second parameter, test of testHelper.

We could just do:
def testHelper(pos: Position = NoPosition, msg: String, severity: String = "")(test: () => Unit)

I think I was trying to avoid repeating the pos info. My second example reads well; you'd only need arrow syntax to ignore the pos in the test function.

But yes, you could just:

testHelper(myPos, "test")(reporter.print(myPos, "test", INFO))

with by-name param instead of a function:

def testHelper(...)(test: => Unit)

Either way, I think it's worth the trouble, because they want to do more tricks with reporters, like filtering, so it's likely your test will get exercised and expanded.

som-snytt · 2017-03-08T17:09:11Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    testHelper(false, false, "test", "")
+
+    reporter.print(noPos, "test", reporter.WARNING)
+    testHelper(false, false, "test", "warning: ")


The false, false, which I've forgotten what it means, is noise.

Sometimes a little table of truth values in code is tidy, but not as args. It can help to name them, f(hasPos = false, isBlank = false, x, y), and in non-test code the repo requires named boolean args for this reason.

Eliminating the noise would take this to another level.

som-snytt · 2017-03-08T17:25:36Z

src/compiler/scala/tools/nsc/reporters/ConsoleReporter.scala

@@ -91,4 +91,4 @@ class ConsoleReporter(val settings: Settings, reader: BufferedReader, writer: Pr
  override def flush() = writer.flush()

  override def finish() = printSummary()
-}
+}


Probably restore terminal NL.

som-snytt · 2017-03-08T17:33:13Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+
+    reporter.WARNING.count = 26
+    reporter.display(posWithSource, "Testing display for maxwarns to fail", reporter.WARNING)
+    testHelper(true, false, "", "")


The one thing tested by test/files/neg/maxerrs.scala is that output is suppressed but the error or warning over the max to display is still counted, and not simply ignored. Here, the count is bumped.

piyush-jaiswal · 2017-03-10T15:34:23Z

I've tried to make the changes. Please review. Thanks!

som-snytt · 2017-03-10T16:22:12Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+      Option(echoWriterOut) match {
+        case None => new ConsoleReporter(new Settings(), reader, writer)
+        case Some(_) => new ConsoleReporter(new Settings(), reader, writer, new PrintWriter(echoWriterOut))
+      }


This is an idiom you'll see nowhere. I won't nitpick default to null, but you can just case null => or just val echo = if (out != null) new PrintWriter(out) else writer. You might as well name the params errOut and echoOut since they aren't Writers.

I think I read somewhere that null was provided in scala for java interop and that Option is better to use in scala. As it may be, I think I'll use null => as it utilizes one of the constructors in ConsoleReporter.

To avoid both null and boxing, method overload: f(out) calls f(out, out). Sometimes the default arg can be an empty value of the desired type.

som-snytt · 2017-03-10T16:30:05Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    testHelper(msg = "test")(_ => reporter.print(NoPosition, "test", reporter.INFO))
+    testHelper(msg = "test", severity = "warning: ")(_ => reporter.print(NoPosition, "test", reporter.WARNING))
+    testHelper(msg = "test", severity = "error: ")(_ => reporter.print(NoPosition, "test", reporter.ERROR))
+    testHelper(posWithSource, msg = "test")(_ => reporter.print(posWithSource, "test", reporter.INFO))


Since your helper takes Pos => Unit, this would be

testHelper(posWithSource, msg = "test")(reporter.print(_, "test", reporter.INFO))

If you find that finicky, then helper could just take (body: => Unit), and maybe that's easier to read, too:

testHelper(posWithSource, msg = "test")(reporter.print(posWithSource, "test", reporter.INFO))

som-snytt · 2017-03-10T16:38:25Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    reporter.ERROR.count += 1
+    testHelper(posWithSource, msg = "Testing display for maxerrs to pass", severity = "error: ")(_ => reporter.display(posWithSource, "Testing display for maxerrs to pass", reporter.ERROR))
+    reporter.ERROR.count += 1
+    testHelper(msg = "")(_ => reporter.display(posWithSource, "Testing display for maxerrs to fail", reporter.ERROR))


Since you're testing display (and not reporter.error), I'd set maxerrs = 1 and ERROR.count = 0 explicitly in the test so we can see the conditions of the test. I don't know what the current count is, does it depend on order tests are run? etc.

som-snytt · 2017-03-10T16:45:09Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+    assertEquals("three warnings found", it.next)
+    assertEquals("10 errors found", it.next)
+    writerOut.reset
+  }


The tests assume test isolation. Probably I wouldn't have used a shared reporter, since it's cheap to construct. Each test could use a throw-away reporter. Here, for instance, the counts are bumped up and other tests could see that, depending on how the test is run (one Test instance per test method or one instance for all, test order, etc).

Should I use a separate BAOS for each test as well and pass it as an argument in testHelper or let it remain a global as it is now?

som-snytt · 2017-03-10T16:49:22Z

Just a couple of small comments to push it across. It's rude at this point to ask not to use a shared reporter across test methods, so I'll leave that to you, so long as the tests actually pass. For future reference, I avoid sharing the "system under test" unless it's expensive to construct.

adriaanm · 2017-03-10T17:19:45Z

Good work, @piyush-jaiswal -- thanks. Thank you for the in-depth reviewing, @som-snytt!

som-snytt · 2017-03-10T17:46:38Z

I'm pretty sure this PR hit all of the hardest problems in computer science, or I might be off by one.

This test will be so useful when someone adds reporter features. 👍

piyush-jaiswal · 2017-03-11T03:27:15Z

Updated. I've used the same BAOS for constructing all the separate reporters ATM.

som-snytt · 2017-03-11T03:42:15Z

test/junit/scala/tools/nsc/reporters/ConsoleReporterTest.scala

+        assertEquals("    ^", it.next)
+      }
+    }
+    writerOut.reset


Technically, we should have put the reset in a finally block. If it fails, it throws, so the next test to run could see the dirty buffer. Probably the test class is reinstantiated; I think junit does that.

som-snytt · 2017-03-11T05:27:58Z

LGTM

piyush-jaiswal · 2017-03-11T20:06:26Z

Thanks @adriaanm and thanks @som-snytt for taking the time to review this PR. This definitely was a great learning experience for me!

no interesting merge conflicts. versions.properties is always a conflict factory, and HashMap.scala was because of parallel collections removal. % export mb=$(git merge-base origin/2.12.x origin/2.13.x) % git log --graph --oneline --decorate $mb..origin/2.12.x | cat * 96cdfb4 (origin/HEAD, origin/2.12.x) Merge pull request scala#5741 from monkey-mas/bump-up-sbt-jmh-to-0.2.21 |\ | * 08f753e Bump up sbt-jmh to 0.2.21 * 38baf2b Merge pull request scala#5771 from som-snytt/issue/regex-doc-err |\ | * cc1b8f3 Fix and improve Regex doc * | 6f905fd Merge pull request scala#5747 from dwijnand/fix-root-package-task |\ \ | * | 4c1c8de Rewire package to osgiBundle for OSGi-enabled projects | / * | 57948c8 Merge pull request scala#5791 from SethTisue/cruft-begone |\ \ | * | 4b98161 remove test/pending directory too | * | 25048bc (SethTisue/cruft-begone) rm -r test/{flaky,disabled*,checker-tests,support,debug} | * | 0563c4b remove orphaned checkfiles | * | 36357f3 remove two empty source files | * | 8690809 fix typos | * | 8177b99 unset a stray execute bit * | | c7c2152 Merge pull request scala#5776 from lrytz/numbersCleanup |\ \ \ | * | | 77e0602 cleanups and clarifications in versions.properties | / / * | | 95a263e Merge pull request scala#5789 from ceeph/patch-1 |\ \ \ | |/ / |/| | | * | a9ba3e6 Fix table formatting |/ / * | 6048c66 Merge pull request scala#5780 from lrytz/bootstrapOverwrite |\ \ | * | 6ff3891 Use a single repository in the bootstrap job |/ / * | 0dab108 Merge pull request scala#5777 from som-snytt/issue/10226 |\ \ | * | c77b420 SI-10226 REPL handles paste when colorized |/ / * | d0c2620 Merge pull request scala#5756 from rorygraves/2.12.x_depthboxing |\ \ | * | f7cd76b Reduce boxing of scala.reflect.internal.Depth | / * | 17e057b Merge pull request scala#5755 from rorygraves/2.12.x_map4 |\ \ | * | 0c6d4e5 Performance improvements for Map4 to HashMap nad Set4 to HashSet transitions | * | aefc8f4 Add benchmarks for Map4 to HashMap and Set4 to HashSet transitions | * | fad8b95 Fix compile error on existing ListBenchmark | / * | 3fadf69 Merge pull request scala#5675 from piyush-jaiswal/issue/9729 |\ \ | |/ |/| | * cc99026 Add tests for ConsoleReporter. * 6e9268b Merge pull request scala#5761 from lrytz/sd329 |\ | * 6abb6ba Don't use `equals` for comparing java.lang.Double/Float * 680d866 Merge pull request scala#5719 from retronym/ticket/10187 |\ | * 898fa00 SI-10187 Support mutation of mutable.HashMap in getOrElseUpdate * 0e38f28 Merge pull request scala#5767 from som-snytt/issue/5621 |\ | * dea2171 SI-5621 Missing implicits are supplied by defaults * 482879f Merge pull request scala#5769 from som-snytt/issue/8969 |\ | * 050811b SI-8969 Accept poly+implicit for assignment syntax * cba77ce Merge pull request scala#5766 from lrytz/versionsReadme * 8fcb9f1 Adapt README to new version numbers % git merge origin/2.12.x % git status ... both modified: src/library/scala/collection/mutable/HashMap.scala deleted by them: test/pending/shootout/fasta.scala both modified: versions.properties % git rm --force test/pending/shootout/fasta.scala % emacs versions.properties % git add -u versions.properties % emacs src/library/scala/collection/mutable/HashMap.scala % git add -u src/library/scala/collection/mutable/HashMap.scala

scala-jenkins added this to the 2.12.2 milestone Feb 4, 2017

som-snytt reviewed Feb 5, 2017

View reviewed changes

SethTisue added the WIP label Feb 6, 2017

som-snytt reviewed Feb 12, 2017

View reviewed changes

som-snytt reviewed Feb 14, 2017

View reviewed changes

adriaanm modified the milestones: 2.12.2, WIP Feb 16, 2017

piyush-jaiswal changed the title ~~Added tests for ConsoleReporter~~ Add increment and reset severity functions, as well as tests for ConsoleReporter. Feb 21, 2017

piyush-jaiswal changed the title ~~Add increment and reset severity functions, as well as tests for ConsoleReporter.~~ Add ability to access severity count in ConsoleReporter and provide tests for the ConsoleReporter. Feb 21, 2017

piyush-jaiswal changed the title ~~Add ability to access severity count in ConsoleReporter and provide tests for the ConsoleReporter.~~ Provide access to severity count in ConsoleReporter and add tests for the ConsoleReporter. Feb 21, 2017

piyush-jaiswal changed the title ~~Provide access to severity count and add tests for ConsoleReporter.~~ Add tests for ConsoleReporter. Mar 1, 2017

som-snytt reviewed Mar 8, 2017

View reviewed changes

som-snytt reviewed Mar 10, 2017

View reviewed changes

Add tests for ConsoleReporter.

cc99026

som-snytt reviewed Mar 11, 2017

View reviewed changes

som-snytt merged commit 3fadf69 into scala:2.12.x Mar 11, 2017

scala-jenkins added the reviewed label Mar 11, 2017

SethTisue removed this from the WIP milestone Mar 6, 2018

SethTisue removed the WIP label May 21, 2018

SethTisue added this to the 2.12.2 milestone May 21, 2018



		def testHelper(checkBlank: Boolean, hasPos: Boolean, message: String, severity: String): Unit = {
		if(checkBlank) assertEquals("", writerOut.toString)

Add tests for ConsoleReporter. #5675

Add tests for ConsoleReporter. #5675

Conversation

piyush-jaiswal commented Feb 4, 2017 • edited

piyush-jaiswal commented Feb 4, 2017

som-snytt commented Feb 5, 2017

piyush-jaiswal commented Feb 5, 2017

piyush-jaiswal commented Feb 5, 2017

som-snytt commented Feb 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-jaiswal commented Feb 5, 2017

som-snytt commented Feb 5, 2017

piyush-jaiswal commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-jaiswal commented Feb 13, 2017

piyush-jaiswal commented Feb 14, 2017

som-snytt 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

piyush-jaiswal commented Feb 18, 2017

som-snytt commented Feb 18, 2017

piyush-jaiswal commented Feb 19, 2017 • edited

adriaanm commented Feb 20, 2017

piyush-jaiswal commented Feb 22, 2017

som-snytt commented Feb 28, 2017

piyush-jaiswal commented Mar 1, 2017

piyush-jaiswal commented Mar 2, 2017

piyush-jaiswal commented Mar 8, 2017

Choose a reason for hiding this comment

som-snytt Mar 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-jaiswal Mar 9, 2017 • edited

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

piyush-jaiswal commented Mar 10, 2017

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

som-snytt commented Mar 10, 2017

adriaanm commented Mar 10, 2017

som-snytt commented Mar 10, 2017

piyush-jaiswal commented Mar 11, 2017

Choose a reason for hiding this comment

som-snytt commented Mar 11, 2017

piyush-jaiswal commented Mar 11, 2017

piyush-jaiswal commented Feb 4, 2017 •

edited

piyush-jaiswal commented Feb 19, 2017 •

edited

som-snytt Mar 8, 2017 •

edited

piyush-jaiswal Mar 9, 2017 •

edited