scala 2.12.10 is default version, closes #703

[ittaiz]

Description

scala 2.12.10 is default version

Motivation

#703

[smparkes]

Not sure if it's overloading too much, but could make the jump to 2.12.11 while we're here?

        "2.12.11": {
            "shas": {
                "scala_compiler": "e901937dbeeae1715b231a7cfcd547a10d5bbf0dfb9d52d2886eae18b4d62ab6",
                "scala_library": "dbfe77a3fc7a16c0c7cb6cb2b91fecec5438f2803112a744cb1b187926a138be",
                "scala_reflect": "5f9e156aeba45ef2c4d24b303405db259082739015190b3b334811843bd90d6a",
            },
        },

[smparkes]

Otherwise LGTM

[ittaiz]

@smparkes test coverage on fails when I apply your change. I'm reverting but I'd welcome a passing PR to upgrade to 2.12.11

[smparkes]

@ittaiz ack

We manually configure 2.12.11 but we haven't run all the tests. Our code is probably less sensitive. I'll see if I can carve out some time.

[ittaiz]

arghh. Even after the revert it fails. I hope it's a travis problem. Though not really sure why only on that test. It's late enough for me to not think straight. I'll try to rerun in the morning and we'll see

[ittaiz]

ok, it seems the coverage failure is legit.
I looked into the passing commit and the travis build didn't run there so that's probably why it passed.

[ittaiz]

I've updated the golden coverage file and so the test passes of course.
It sounds reasonable that if we upgraded the libraries then the coverage file might have changed format. @gergelyfabian @smparkes would still love your 2c about this if it indeed sounds reasonable since I'm always ware when changing tests

[gergelyfabian]

I think there is some strange change with coverage on this branch.

Steps:

1. Generate coverage data:
   bazel coverage --extra_toolchains="//scala:code_coverage_toolchain" //test/coverage/...
2. Generated coverage report:
   genhtml -o ${destdir} --ignore-errors source bazel-out/k8-fastbuild/testlogs/test/coverage/test-all/coverage.dat

The report on master and on 2_12_default is significantly different.
E.g. in coverage/D1.scala.gcov.html all the function lines seem to be covered on master, while on 2_12_default only the first line is shown as covered.
On master, line coverage (for the whole .dat file) is 91.1%, while on 2_12_default it's 85.7%.

I checked this, because I got suspicious when I saw the differences in the expected lcov file (removing mentions of veryLongFunctionNameIsHereAaaaaaaaa).

[ittaiz]

:( always be cautious when changing tests...

@gergelyfabian thanks for taking the time!
How do you think we should proceed?

[gergelyfabian]

Results of lcov --list bazel-out/k8-fastbuild/testlogs/test/coverage/test-all/coverage.dat on master:

Reading tracefile bazel-out/k8-fastbuild/testlogs/test/coverage/test-all/coverage.dat
                   |Lines       |Functions  |Branches    
Filename           |Rate     Num|Rate    Num|Rate     Num
=========================================================
[test/coverage/]
A1.scala           |66.7%      3|75.0%     4|    -      0
A2.scala           | 100%      2|75.0%     4|    -      0
B1.scala           |50.0%      2|50.0%     4|    -      0
B2.java            |25.0%      4|33.3%     3|    -      0
C2.scala           | 100%      2| 100%     4|    -      0
D1.scala           | 100%     36|98.6%    70|    -      0
TestAll.scala      | 100%      7| 100%     9|    -      0
=========================================================
             Total:|91.1%     56|92.9%    98|    -      0

On 2_12_default:

Reading tracefile bazel-out/k8-fastbuild/testlogs/test/coverage/test-all/coverage.dat
                   |Lines       |Functions  |Branches    
Filename           |Rate     Num|Rate    Num|Rate     Num
=========================================================
[test/coverage/]
A1.scala           | 100%      4|75.0%     4|    -      0
A2.scala           | 100%      3|75.0%     4|    -      0
B1.scala           |66.7%      3|50.0%     4|    -      0
B2.java            |25.0%      4|33.3%     3|    -      0
C2.scala           | 100%      3| 100%     4|    -      0
D1.scala           | 100%      4|75.0%     4|    -      0
TestAll.scala      | 100%      7| 100%     1|    -      0
=========================================================
             Total:|85.7%     28|70.8%    24|    -      0

[gergelyfabian]

@ittaiz I'm not an expert on lcov data generation, but maybe there is a difference how Scala 2.12 classes are generated / instrumented. I believe this would need some debugging, because evidently lines which are executed are not detected by coverage.

[ittaiz]

Coming to think about it I’m not sure we should block the merge because of this.
I think we found a problem with coverage on 2.12 which we should open an issue for and then hopefully @gergelyfabian can iterate on it while master is on 2.12, right?
If we merge this it doesn’t worsen the state of those using rules scala with coverage on 2.12

[ittaiz]

@gergelyfabian @smparkes wdyt?

[gergelyfabian]

@ittaiz If Scala 2.12 was already available, and this is only about changing the default, then I am not against, although I am not sure whether and when I would have time to check this bug (and whether I have the knowledge ;)). Ittai Zeidman <[email protected]> ezt írta (időpont: 2020. jún. 10., Sze 20:06):

…

@gergelyfabian <https://github.com/gergelyfabian> @smparkes <https://github.com/smparkes> wdyt? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1054 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJGRRMYMUBWKLCYXKT7QADRV7DRRANCNFSM4NTP5BSQ> .

[smparkes]

I wouldn't block on coverage.

I would say coverage accuracy is a minor issue in terms of which version of a compiler to use. In a case like this, there are far more significant issues that impact the decision to use 2.11 or 2.12.

It's good that we have a test but I don't think we're trying to fix the coverage tools themselves, just to make sure they're being run correctly. So run once, record the results, etc. This is always going to be a bit brittle since coverage could change by compiler patch version. But we're not trying to test every version and the focus is on running the tool correctly so ...