non deterministic failure for test add_override_final_flags_to_lazy_val

[retronym]

Results :

Failed tests: 
  ReusingPrinterTest.add_override_final_flags_to_lazy_val expected:<...T extends T {
      [override final ]lazy val meth = 0
  ...> but was:<...T extends T {
      []lazy val meth = 0
  ...>

https://scala-webapps.epfl.ch/jenkins/job/pr-scala/8915/

The test relates to 087f719.

[retronym]

/cc @sschaef

[misto]

Thanks for reporting! I'm looking into it:
while mvn -P scala-2.11.x -Dtest='ReusingPrinterTest#add_override_final_flags_to_lazy_val' test; do; done

[misto]

Argh, can't reproduce it. I tried with different scala versions (2.11.0, 2.11.4, 2.11.5-SNAPSHOT), but it hasn't failed yet.

[kiritsuku]

I don't think it is easy to reproduce such a failure. I think it depends on which other tests are executed beforehand without restarting the compiler in the meantime. The easiest (and probably only) fix for now is to retry the failing tests, something similar to what exists in scala-ide with FlakyTest.

[retronym]

Do you have any hypotheses for the source of flakiness? What is the threading model of the tests + scala-refactoring?

I've seen this particular test fail in PR validation of scala/scala#4036 a number of times.

[retronym]

Does the total run time of this failure look normal or abnormal? I'm wondering if load on the machines is a trigger.

[kiritsuku]

I have no clue about what could be the reason. On my system, an entire build takes only 1m 25s.

[misto]

I don't know how fast the machine is, but 7 minutes seems rather long. Scala-refactoring doesn't use any multi-threading, and operations should be run on the PC thread, that's all.

[misto]

A typical source of trouble is that we re-use a single instance of the compiler for all the tests, otherwise it's just too slow. That's also why the tests in that file all declare a package, e.g. add_override_final_flags_to_lazy_val, to at least somewhat isolate them from each other.

[misto]

@sschaef to me it looks like d.symbol.isLazy returns false when it shouldn't, and then the transformation isn't performed. Could we rewrite the tests so that it doesn't use isLazy?

[kiritsuku]

One could check !d.symbol.isAbstract or d.owner.name.toString == "TT" to get the relevant method but if that solves the problem is questionable.

[misto]

I vaguely remember that I've had problems with isLazy in tests in the past. But yeah, a proper solution would be better!

[kiritsuku]

@retronym Maybe the problem is now fixed, we don't know. Please reopen this ticket if you see that this problem still happens, I will implement a better fix then.

[retronym]

My hypothesis is that the cross-talk between tests comes from the reporter.

In typedTemplate, we see that derived trees, which include lazy val getters, are not included in the tree if the reporter has any errors.

      val body2 = {
        val body2 =
          if (isPastTyper || reporter.hasErrors) body1
          else body1 flatMap rewrappingWrapperTrees(namer.addDerivedTrees(Typer.this, _))

I can reproduce the test failure by provoking this situation:

@Test
  def add_override_final_flags_to_lazy_val_after_error() = {
    println("calling add_override_protected_abstract_flag")
    add_override_protected_abstract_flag()
    println("reporting an error")
    global.reporter.error(NoPosition, "")
    println("calling add_override_final_flags_to_lazy_val")
    add_override_final_flags_to_lazy_val()
  }

calling add_override_protected_abstract_flag
reporting an error
calling add_override_final_flags_to_lazy_val

org.junit.ComparisonFailure:  <Click to see difference>

The reporter is reset inside interactive.Global#backgroundCompile, but this isn't done automatically before processing a work item found by pollForWork.

The logic in the Typer seems too crude for the presentation compiler. I expect it exists to avoid cascading errors (or crashes), but ideally we could be more fine grained.

What is your expectation about the state of the reporter? before each test. I added if (global.reporter.hasErrors) println("errors detected") in the @Before def resetPresentationCompiler and indeed the reporter is not cleaned up.

I would suggest adding global.reporter.reset() in there. You could also throw in global.analyzer.resetTyper() for good measure.

/cc @dragos

[retronym]

If this hypothesis is correct, the workaround commited won't work: the isLazy test isn't failing, but rather the entire lazy val getter DefDef is absent from the tree.

[misto]

I'm reopening this so that we don't forget to revert our initial quick fix.