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

[ELY-2480] Add code coverage to the project #1941

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

Dkafetzis
Copy link

https://issues.redhat.com/browse/ELY-2480

Added a new profile in maven to run test coverage for the elytron project. The profile name is "test-coverage"
Running the following will run the test coverage
mvn clean install -Ptest-coverage

@Skyllarr
Copy link
Contributor

Hi @Dkafetzis , thanks a lot for the PR!
I have a question, I can see in the resulting file at path/to/wildfly-elytron/target/jacoco-ut/org.wildfly.security.http.basic/index.html that the coverage for BasicAuthenticationMechanism is 0% . We have tests for this mechanism in the module tests/base. Is that something that can be taken into account when generating the report?

@Dkafetzis
Copy link
Author

Hello @Skyllarr, Yes i see now that some tests are not included in the coverage report. I'll look further into this, any suggestions on this are welcome.

@ivassile
Copy link
Contributor

ivassile commented Feb 27, 2024

@Dkafetzis I tested this change using the latest 2.x code and I'm getting the following exception:

[INFO] Instrumentation error
com.atlassian.clover.api.CloverException: wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
    at com.atlassian.clover.instr.java.Instrumenter.instrument (Instrumenter.java:160)
    at com.atlassian.clover.CloverInstr.execute (CloverInstr.java:110)
    at com.atlassian.clover.CloverInstr.mainImpl (CloverInstr.java:86)
....

[ERROR] Failed to execute goal org.openclover:clover-maven-plugin:4.5.2:setup (clover-setup) on project wildfly-elytron-sasl: Clover has failed to instrument the source files in the [wildfly-elytron/sasl/base/target/clover/src-instrumented] directory -> [Help 1]

As a suggestion, we can use the latest version of OpenClover - 4.5.2

@Dkafetzis
Copy link
Author

I updated it to the latest version. It works now on my machine, but feel free to try it out. Also try running it with mvn clean install -Ptest-coverage org.openclover:clover-maven-plugin:aggregate org.openclover:clover-maven-plugin:clover

@ivassile
Copy link
Contributor

@Skyllarr Can you also test this change? I'm still getting the below exception in my environment. Thanks!

mvn clean install -Ptest-coverage org.openclover:clover-maven-plugin:aggregate org.openclover:clover-maven-plugin:clover

[INFO] --- clover:4.5.2:setup (clover-setup) @ wildfly-elytron-sasl ---
[INFO] OpenClover Version 4.5.2, built on 2024-01-31
[INFO] Creating new database at '/Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/target/clover/clover.db'.
[INFO] Processing files at JAVA_11 source level.
[INFO] /Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
[INFO] Instrumentation error
com.atlassian.clover.api.CloverException: /Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
    at com.atlassian.clover.instr.java.Instrumenter.instrument (Instrumenter.java:160)
    at com.atlassian.clover.CloverInstr.execute (CloverInstr.java:110)

@fjuma
Copy link
Contributor

fjuma commented Mar 14, 2024

@Skyllarr Can you also test this change? I'm still getting the below exception in my environment. Thanks!

mvn clean install -Ptest-coverage org.openclover:clover-maven-plugin:aggregate org.openclover:clover-maven-plugin:clover

[INFO] --- clover:4.5.2:setup (clover-setup) @ wildfly-elytron-sasl ---
[INFO] OpenClover Version 4.5.2, built on 2024-01-31
[INFO] Creating new database at '/Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/target/clover/clover.db'.
[INFO] Processing files at JAVA_11 source level.
[INFO] /Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
[INFO] Instrumentation error
com.atlassian.clover.api.CloverException: /Users/ivassile/Downloads/ely/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
    at com.atlassian.clover.instr.java.Instrumenter.instrument (Instrumenter.java:160)
    at com.atlassian.clover.CloverInstr.execute (CloverInstr.java:110)

@ivassile This worked for me locally.

pom.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks @Dkafetzis! Have added a couple small comments.

pom.xml Outdated Show resolved Hide resolved
@Dkafetzis
Copy link
Author

@fjuma Can I also get a review on this one?

@ivassile
Copy link
Contributor

ivassile commented May 1, 2024

@fjuma When you have a chance, can you review the requested changes? Thanks!

pom.xml Outdated Show resolved Hide resolved
@fjuma
Copy link
Contributor

fjuma commented May 1, 2024

@Dkafetzis Thanks very much for the updates, added a small comment but other than that, it looks good! Thanks very much!

@Dkafetzis
Copy link
Author

@fjuma Indentation fixed, thanks for the correction.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks @Dkafetzis!

@fjuma fjuma added the +1 FJ label May 7, 2024
@ivassile
Copy link
Contributor

@Dkafetzis Can you please rebase? Thanks!

@Dkafetzis Dkafetzis force-pushed the ELY-2480 branch 2 times, most recently from b1af876 to a75c959 Compare August 26, 2024 08:28
@Dkafetzis
Copy link
Author

@ivassile Done, sorry for the delay :)

@darranl
Copy link
Contributor

darranl commented Sep 15, 2024

I was just trying to build with coverage before merging but unfortunately I get this error locally:

[INFO] /home/darranl/src/2024/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
[INFO] Instrumentation error                                                                              
com.atlassian.clover.api.CloverException: /home/darranl/src/2024/wildfly-elytron/sasl/base/src/main/java/org/wildfly/security/sasl/SaslMechanismSelector.java:432:45:unexpected token: switch
    at com.atlassian.clover.instr.java.Instrumenter.instrument (Instrumenter.java:160)                
    at com.atlassian.clover.CloverInstr.execute (CloverInstr.java:110)                          
    at com.atlassian.clover.CloverInstr.mainImpl (CloverInstr.java:86)                         
    at com.atlassian.maven.plugin.clover.internal.instrumentation.AbstractInstrumenter.instrumentSources (AbstractInstrumenter.java:205)                                                                                 at com.atlassian.maven.plugin.clover.internal.instrumentation.AbstractInstrumenter.instrument (AbstractInstrumenter.java:80)
    at com.atlassian.maven.plugin.clover.CloverInstrumentInternalMojo.execute (CloverInstrumentInternalMojo.java:278)
    at com.atlassian.maven.plugin.clover.CloverSetupMojo.execute (CloverSetupMojo.java:31)                
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:126)
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute2 (MojoExecutor.java:328)                                                                                                                           
    at org.apache.maven.lifecycle.internal.MojoExecutor.doExecute (MojoExecutor.java:316)                              

@darranl
Copy link
Contributor

darranl commented Sep 15, 2024

@Dkafetzis thank you for the work so far, FYI once we get this in it could be a good candidate to have a weekly GitHub action that generates this report so we can monitor (but that would be a follow up task, not part of this PR)

@fjuma fjuma removed the +1 FJ label Oct 2, 2024
@fjuma fjuma added the fixme label Oct 2, 2024
@Dkafetzis
Copy link
Author

@fjuma @darranl Sorry for the long delay, other more urgent issues popped up. This empty switch statement seems to be throwing off openclovers Instrumenter, can we refactor it?

@fjuma
Copy link
Contributor

fjuma commented Nov 7, 2024

Feel free to refactor the empty switch statement.

@Dkafetzis
Copy link
Author

@fjuma The switch statement has been refactored and the coverage report gets generated with the command mvn -Ptest-coverage clean org.openclover:clover-maven-plugin:setup install org.openclover:clover-maven-plugin:aggregate org.openclover:clover-maven-plugin:clover successfully. I have also changed the notes to reflect the change in the command.

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

Successfully merging this pull request may close these issues.

5 participants