Fix #537 - support warnings that span multiple lines in standalone compiler #543
Conversation
problem with guice
We need to update the config for the new tests in org.lflang.lfc
There was a problem hiding this comment.
Very nice feature! It's also a great step forward to have tests for this. The only thing I worry about is the reporting to CodeCov. What I expect to happen is that whichever job finishes last will report it's coverage as overall coverage. I might be wrong, because I'm not completely sure how this works, but we should be on the look out for this...
|
There's also this issue: #336 |
|
Why don't we make the tests you added part of the compiler tests? Is it because of Kotlin? We should really find a way to calculate code coverage over the entire code base... |
|
Maybe this helps? https://docs.codecov.com/docs/merging-reports |
|
Judging by what is described there, reporting is incremental by default, so it might actually just work the way you were doing it initially... Let's just give it a shot and see what happens? |
|
@lhstrh Our codecov integration looks broken anyway, this should probably be the focus of another fix PR.
Maybe this data is private and I need to log in? |
|
RE: CodeCov: The move from the |
There was a problem hiding this comment.
This looks good! Thanks for taking care of this and also adding tests.
Why don't we make the tests you added part of the compiler tests? Is it because of Kotlin? We should really find a way to calculate code coverage over the entire code base...
I think it is a good idea to keep tests close to the source code. I am not sure how this is commonly handled in Java projetcs, but in Python projects it's quite common to have tests in every package. The tests here test lfc's ability to produce correctly formatted error messages. This shouldn't be a concern relevant to any other package.
Also, we need to be careful about dependencies here. Adding dependencies to Kotlin code from projects that we can compile from Eclipse will cause trouble. The org.lflang.lfc project is currently not even visible to our developer Eclipse.
| @@ -61,7 +61,7 @@ | |||
| private static String MAIN_PATH_IN_JAR = String.join("/", | |||
| new String[] {"!", "org", "lflang", "lfc", "Main.class"}); | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
@lhstrh We really need to decide on a formatter and consistent style that we use across IDEs. All these formatting changes make it really hard to review things
I fully agree. The typical gradle setup is that each subproject has both main and test sources (like here for the submodule |
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
=========================================
Coverage ? 66.39%
Complexity ? 3465
=========================================
Files ? 132
Lines ? 22321
Branches ? 2886
=========================================
Hits ? 14819
Misses ? 6354
Partials ? 1148 Continue to review full report at Codecov.
|
|
Looks like the stack trace option kills code coverage? |
I'm not sure what you mean, but I think the reason we got a coverage report is that that commit is the first to this branch since your changes to code coverage reporting on master. I don't think it's related to the stack trace option itself |
|
Yes, you're right. What about the test failures in the standalone tests though? |
It seems to be failing only on windows, I'll need some time to reproduce it :'( |
|
Looks like this Windows failure is the only thing holding us back from merging this. Any progress on this issue? |
|
I think this is done, I'm going to merge it |
This also adds tests for the way the standalone compiler formats messages. (that required changes in CI files, there's a new workflow standalone-lfc-tests)
This is a sample multiline warning:
If the block is too long it's shortened with
...