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.

Sign up for GitHub

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

Jump to bottom

[CI] Enable compatibility test matrix for unit tests #1915

Merged
merged 13 commits into from
Aug 2, 2021
Merged

[CI] Enable compatibility test matrix for unit tests #1915

merged 13 commits into from
Aug 2, 2021

Conversation

v1v
Copy link
Member

@v1v v1v commented Jul 12, 2021 •
edited

What does this PR do?

Run the compatibility unit tests in the CI on a weekly basis for the master branch.

On a PR basis can be enabled with:

  • a GitHub comment -> run compatibility tests or jenkins run the compatibility tests or run the compatibility tests
  • using the UI and click on the compatibility_ci parameter.

Why

To get towards supporting more recent versions officially, we should add Java 12-16 to the test matrix.

Important

java12 is the latest java version for that flavour so, there are different ones that could be used in jdk >12:

  • openjdk
  • zulu
  • adoptopenjdk

This unit test matrix will run always after the APM ITs, for simplicity, I didn't add more complexity in the pipeline to run more things in parallel.

Pipeline UI

image

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else

@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Jul 12, 2021
@v1v
Copy link
Member Author

v1v commented Jul 12, 2021

jenkins run the matrix tests

@apmmachine
Copy link
Collaborator

apmmachine commented Jul 12, 2021 •
edited

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-02T07:08:04.638+0000

  • Duration: 57 min 27 sec

  • Commit: f8daa6a

Test stats 🧪

Test Results
Failed 0
Passed 2378
Skipped 19
Total 2397

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2378
Skipped 19
Total 2397

@v1v
Copy link
Member Author

v1v commented Jul 12, 2021

jenkins run the matrix tests

@v1v v1v marked this pull request as ready for review July 12, 2021 17:42
@v1v v1v requested review from a team July 12, 2021 17:42
@v1v v1v added automation Tests & automation that help build & maintain the project Team:Automation Label for the Observability productivity team labels Jul 12, 2021
@v1v v1v self-assigned this Jul 12, 2021
cachedout
cachedout approved these changes Jul 13, 2021
View reviewed changes
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

👍 Nice work @v1v

@v1v
Merge branch 'master' into feature/add-java-junit-support
5420a0c 
* master:
  Bump version.slf4j from 1.7.31 to 1.7.32 (elastic#1933)
  Add description about memory pool metrics to docs (elastic#1925)
  updated team membership check in action
  Add 1.25.0 to cloudfoundry index
  Update CHANGELOG.asciidoc (elastic#1929)
  fixed community label action
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release v1.25.0
  Prepare release 1.25.0 (elastic#1927)
  synchronize json schema specs (elastic#1926)
  added labeling of community issues and PRs
  added community labeler config
  Upgrade Byte Buddy to 1.11.8 (elastic#1923)
  Bump json-schema-validator from 1.0.56 to 1.0.57 (elastic#1916)
  Bump log4j2-ecs-layout from 1.0.1 to 1.1.0 (elastic#1917)
  Added support for setting the framework using the public api (elastic#1908) (elastic#1909)
  Add Javalin plugin (elastic#1822)
@SylvainJuge
Copy link
Member

Good job !

Do you know if there is a way to set the stage name dynamically on the matrix axis value ?
From what I understand, the stage name is not interpolated in a declarative pipeline, thus we can't have

stage("${JAVA_VERSION}"){
  // ...
}

While not being something blocking, that would definitely be great and help spot failures, we have the Java version visible when the cursor is over the Matrix ..., but it's definitely not obvious.

@v1v
Copy link
Member Author

v1v commented Jul 28, 2021

jenkins run the matrix tests

@v1v
Copy link
Member Author

v1v commented Jul 28, 2021

Do you know if there is a way to set the stage name dynamically on the matrix axis value ?

As far as I'm aware that's a limitation with the declarative pipeline, though, the GitHub checks should provide those details:

image

and also the GitHub comment:
image

I'll include the jdk version in the script therefore the steps errors in the GH comment can also show that:

image

v1v
v1v commented Jul 28, 2021
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
@v1v
Copy link
Member Author

v1v commented Jul 28, 2021

run the matrix tests

@v1v
Copy link
Member Author

v1v commented Jul 28, 2021

Recent changes provide a better UI in the PR itself:

image

@SylvainJuge
Copy link
Member

The matrix testing works properly, however there are plenty of tests that fail and need investigation.

Thus, if we merge it to master as is, it means our build will be broken for a while.

  • can we split the run as master option and have a dedicated boolean option for those compatibility tests ?
  • can we execute those tests once in a while, for example weekly so we don't get them on every build on master which is triggered on every PR merge ?

@SylvainJuge
Copy link
Member

SylvainJuge commented Jul 29, 2021 •
edited

Here is the non-exhaustive list of bugs that have been found on Jdk 15 and 16:

Thus as we won't be able to fix those in the next few weeks, having a separate option than the run as main to trigger those tests would be required.

@v1v
Copy link
Member Author

v1v commented Aug 2, 2021 •
edited

can we split the run as master option and have a dedicated boolean option for those compatibility tests ?

Yes, I'll enable a new parameter called compatibility-ci, disabled by default

can we execute those tests once in a while, for example weekly so we don't get them on every build on master which is triggered on every PR merge ?

Yes, I'll configure a weekly run with the compatibility-ci parameter to be set.

Therefore, this feature will never run by default for the time being on branches/tags/PRs. But still PRs can be tested with the GitHub comment (see #1915 (comment)) or the UI parameter (compatibility-ci)

Does it sound like a good plan?

v1v added 4 commits August 2, 2021 07:59
@v1v
Merge remote-tracking branch 'upstream/master' into feature/add-java-…
ed364ff 
…junit-support

* upstream/master:
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
@v1v
Use compatibility rather than matrix
6153df5
@v1v
Run on a weekly basis
eeeb208
@v1v
Master branch
f8daa6a
@v1v v1v changed the title [CI] Enable test matrix for unit tests [CI] Enable compatibility test matrix for unit tests Aug 2, 2021
@SylvainJuge
Copy link
Member

Thanks @v1v , that's definitely a good plan.

We might also reuse a similar strategy for the tests on Windows, which should likely be run on a weekly basis.

@SylvainJuge SylvainJuge merged commit b15c460 into elastic:master Aug 2, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Aug 2, 2021
@v1v v1v deleted the feature/add-java-junit-support branch August 2, 2021 09:26
v1v added a commit to v1v/apm-agent-java that referenced this pull request Aug 2, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/pipeline-…
178844e 
…for-windows-only

* upstream/master: (100 commits)
  [CI] Enable compatibility test matrix for unit tests (elastic#1915)
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
  Bump version.slf4j from 1.7.31 to 1.7.32 (elastic#1933)
  Add description about memory pool metrics to docs (elastic#1925)
  updated team membership check in action
  Add 1.25.0 to cloudfoundry index
  Update CHANGELOG.asciidoc (elastic#1929)
  fixed community label action
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release v1.25.0
  Prepare release 1.25.0 (elastic#1927)
  synchronize json schema specs (elastic#1926)
  added labeling of community issues and PRs
  added community labeler config
  ...
@v1v v1v mentioned this pull request Aug 2, 2021
Merged
@cachedout
Copy link
Contributor

Looks like merging this might have triggered the full run because the parameter wasn't yet present. I'm going to re-run the master branch to ensure this is working as-expected now that the the parameter is in place.

@cachedout
Copy link
Contributor

cachedout commented Aug 2, 2021 •
edited

@v1v The run I kicked off appears to have still run these tests. Could this line be the cause?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cachedout cachedout cachedout approved these changes

@SylvainJuge SylvainJuge SylvainJuge approved these changes

@kuisathaverat kuisathaverat kuisathaverat approved these changes

Assignees

@v1v v1v

Labels
agent-java automation Tests & automation that help build & maintain the project Team:Automation Label for the Observability productivity team
Projects
APM-Agents (OLD)
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@v1v @apmmachine @SylvainJuge @cachedout @kuisathaverat