Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix kotlin 1.8 warnings not detected #925

Merged

Conversation

TVDiasDominik
Copy link
Contributor

with kotlin 1.8 they changed the compile warnings style.
Old style <package-name><filename>.kt: (line,column)
new style <package-name><filename>.kt:line:column
to archive this we need to make the (, )˙ and [, ]optional as well ad a new separator:`

Testing done

Submitter checklist

with kotlin 1.8 they changed the warnings compile output style from the java style to kotlin from
<package-name><filename>.kt: (line,column) to 
<package-name><filename>.kt:line:column
test that the new and old style is still working
add test for the kotlin 1.8 warnings style
@TVDiasDominik TVDiasDominik force-pushed the fix_kotlin_1_8_warnings_not_detected branch from a7eee80 to 173fb1b Compare June 21, 2023 14:25
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #925 (cd51556) into master (0c8517a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #925   +/-   ##
=========================================
  Coverage     92.89%   92.89%           
  Complexity     2322     2322           
=========================================
  Files           341      341           
  Lines          6450     6450           
  Branches        669      669           
=========================================
  Hits           5992     5992           
  Misses          262      262           
  Partials        196      196           
Impacted Files Coverage Δ
...hafner/analysis/parser/AbstractMavenLogParser.java 95.65% <ø> (ø)
...ava/edu/hm/hafner/analysis/parser/JavacParser.java 94.73% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@uhafner uhafner added the enhancement Enhancement of existing functionality label Jun 30, 2023
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the regexp should work now. Can you please add some assertions for the properties of the two warnings, then we are ready to go...

@TVDiasDominik
Copy link
Contributor Author

I think the regexp should work now. Can you please add some assertions for the properties of the two warnings, then we are ready to go...

Hey i tried that here:
assertThat(warnings.get(0)).hasSeverity(Severity.WARNING_NORMAL).hasLineStart(214).hasColumnStart(35); assertThat(warnings.get(1)).hasSeverity(Severity.WARNING_NORMAL).hasLineStart(424).hasColumnStart(29);
but it is not working the first check works fine as it is the old style.
The second check does not work as the filename is including the 424 so it fails with that 29 is 424.
I think it is the regex again and that in group 1 the line is included for the new style as this is not the case for the old style.
I did not found a solution for it that works for Windows paths and Unix paths do you have an idea (I'm no regex expert here)
Expecting lineStart of: <Activity.kt:424(29,0): to be: <424> but was: <29>

@uhafner
Copy link
Member

uhafner commented Jul 5, 2023

Can you please push those changes in the test case? Then I an try to refactor the code.

(In the end we might need to create a new separate parser for Kotlin as the logs are somewhat different.)

@TVDiasDominik
Copy link
Contributor Author

Can you please push those changes in the test case? Then I an try to refactor the code.

(In the end we might need to create a new separate parser for Kotlin as the logs are somewhat different.)

done, yeah i was also thinking about that approach.

@uhafner
Copy link
Member

uhafner commented Jul 8, 2023

Can you please push those changes in the test case? Then I an try to refactor the code.
(In the end we might need to create a new separate parser for Kotlin as the logs are somewhat different.)

done, yeah i was also thinking about that approach.

I tried to tweak the regexp even more but it looks like the format is too different. I think it makes sense to introduce a new Kotlin parser that can parse only Kotlin messages.

@TVDiasDominik
Copy link
Contributor Author

TVDiasDominik commented Jul 9, 2023

Can you please push those changes in the test case? Then I an try to refactor the code.
(In the end we might need to create a new separate parser for Kotlin as the logs are somewhat different.)

done, yeah i was also thinking about that approach.

I tried to tweak the regexp even more but it looks like the format is too different. I think it makes sense to introduce a new Kotlin parser that can parse only Kotlin messages.

but even with a separate kotlin parser the filename group problem will be the same i think.
At least i don't see any fix for this if windows has the c:\..\Activity.kt:12:12 as we need to seperate for windows after the second : and for unix we separate after the first : or am i missing something?
For < Kotlin 1.8 the format is the same as Java only >= 1.8 it changed so the new KotlinParser would also need to support the old format or somewhere there needs to be a differentiation that kotlin >= 1.8 is used and it should use the new KotlinParser

@uhafner
Copy link
Member

uhafner commented Jul 9, 2023

Can you please push those changes in the test case? Then I an try to refactor the code.
(In the end we might need to create a new separate parser for Kotlin as the logs are somewhat different.)

done, yeah i was also thinking about that approach.

I tried to tweak the regexp even more but it looks like the format is too different. I think it makes sense to introduce a new Kotlin parser that can parse only Kotlin messages.

but even with a separate kotlin parser the filename group problem will be the same i think. At least i don't see any fix for this if windows has the c:\..\Activity.kt:12:12 as we need to seperate for windows after the second : and for unix we separate after the first : or am i missing something? For < Kotlin 1.8 the format is the same as Java only >= 1.8 it changed so the new KotlinParser would also need to support the old format or somewhere there needs to be a differentiation that kotlin >= 1.8 is used and it should use the new KotlinParser

I think (*):\d+:\d+ should work.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you did find a solution without creating a new parser 🚀

@uhafner uhafner merged commit fd99fb1 into jenkinsci:master Jul 13, 2023
25 checks passed
@TVDiasDominik
Copy link
Contributor Author

It seems that you did find a solution without creating a new parser 🚀

yeah it took quite some time to find a solution.
Can we do a release here and in the other repo with the changes from here so we can get for the new kotlin version warnings again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants