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

Separate static analyzer task/javadoc from test execution #4852

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

reinhapa
Copy link
Member

@reinhapa reinhapa commented Apr 4, 2023

Combines Test & JavaDoc Workflow to one separating single tasks as follows:

  • Licence check
  • OWASP Scan
  • Java Doc
  • Test matrix OS for test executions only

@line-o line-o requested review from adamretter, dizzzz, duncdrum and line-o and removed request for adamretter and dizzzz April 5, 2023 14:29
Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

This improves our current workflow. Thank you!

@line-o
Copy link
Member

line-o commented Apr 5, 2023

Should be rebased onto current develop after #4853 was merged.

@line-o
Copy link
Member

line-o commented Apr 5, 2023

I would rename the workflow from "Test & documentation" to "Analyse & Test"

@sonarcloud
Copy link

sonarcloud bot commented Apr 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@duncdrum duncdrum merged commit 0b1ef35 into eXist-db:develop Apr 6, 2023
@reinhapa reinhapa deleted the githubactions branch April 6, 2023 06:04
Copy link
Member

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Some good changes, but the separate javadoc CI job doesn't make sense as it duplicates the majority of the work that has to be done. Also a couple of small issues that I have highlighted.

distribution: temurin
java-version: ${{ env.DEV_JDK }}
cache: 'maven'
- run: mvn -V -B -q -T 2C install javadoc:javadoc -DskipTests -D'dependency-check.skip' -D'license.skip' --projects '!exist-distribution,!exist-installer' --also-make
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it makes sense to have the javadoc in a separate task.
As it is setup in this PR, the javadoc CI job will also call these Maven phases:

  1. validate
  2. initialize
  3. generate-sources
  4. process-sources
  5. generate-resources
  6. process-resources
  7. compile
  8. process-classes
  9. generate-test-sources
  10. process-test-sources
  11. generate-test-resources
  12. process-test-resources
  13. test-compile
  14. process-test-classes
  15. test
  16. prepare-package
  17. package
  18. pre-integration-test
  19. integration-test
  20. post-integration-test
  21. verify
  22. install

Only phases 15 and 19, i.e. test and integration test will be reduced (but not omitted) in this job due to the -DskipTests short-cutting the process methods of the maven-surefire-plugin and maven-failsafe-plugin. As all of these phasse are already performed in the Maven Test CI job, separating this into a separate job has increased the amount of work required by the CI due to duplication of phases.

I think it would be better to reinstate it into the Maven Test CI job with a condition that it is only executed on the Linux runner (as that is the fastest runner).

Copy link
Member

Choose a reason for hiding this comment

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

would this mean no unit tests anymore on windows/macos?

Copy link
Member

Choose a reason for hiding this comment

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

No, it would mean that only the javadoc task would be run on Linux.

strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest, macOS-latest]
jdk: ['17']
jdk: [ '17' ]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be - ${{ env.DEV_JDK }} ?

exist-parent/pom.xml Show resolved Hide resolved
@@ -21,9 +59,9 @@ jobs:
cache: 'maven'
- name: Maven Test
timeout-minutes: 60
run: mvn -V -B -DtrimStackTrace=false -D"maven.resolver.transport"="wagon" clean verify
run: mvn -V -B verify -D'dependency-check.skip' -D'license.skip'
Copy link
Member

Choose a reason for hiding this comment

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

Can we reinstate -DtrimStackTrace=false please?

adamretter added a commit to evolvedbinary/exist that referenced this pull request Apr 9, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request Apr 9, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request Apr 9, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 11, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 11, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 11, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 12, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 12, 2023
adamretter added a commit to evolvedbinary/exist that referenced this pull request May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants