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

Run tests against Jackson 2.12 to ensure runtime compatibility #695

Merged
merged 2 commits into from
Nov 7, 2021

Conversation

philsttr
Copy link
Collaborator

@philsttr philsttr commented Nov 7, 2021

Previously, I merged #668, which bumps jackson to 2.13.0.

In this PR, I have added an additional GitHub workflow step that will run tests against jackson 2.12.5 (without recompiling). Running these tests ensures that even though logstash-logback-encoder is compiled against jackson 2.13.0, logstash-logback-encoder continues to work with jackson 2.12.5 at runtime. If users need jackson 2.12.5, they can use dependencyManagement to pin jackson back to 2.12.5.

Note that logstash-logback-encoder is not compatible with jackson 2.11.x (the tests fail due to java.lang.NoClassDefFoundError: com/fasterxml/jackson/core/filter/TokenFilter$Inclusion)

I could not find a way to run the tests against both jackson 2.13.0 and 2.12.5 in a single maven command execution. So, I had to add an additional maven execution. Unfortunately, this means that during development, maven won't run the tests against 2.12 by default. We can either execute them manually pre-commit. Or push and wait for GitHub to tell us if they pass. Any other ideas welcome.

@philsttr philsttr requested a review from brenuart November 7, 2021 18:36
@philsttr philsttr added this to the 7.0 milestone Nov 7, 2021
@brenuart
Copy link
Collaborator

brenuart commented Nov 7, 2021

Note that logstash-logback-encoder is not compatible with jackson 2.11.x (the tests fail due to java.lang.NoClassDefFoundError: com/fasterxml/jackson/core/filter/TokenFilter$Inclusion)

True. This is caused by
AbstractJsonPatternParser.java#L500. I explicitly used the constructor introduced in Jackson 2.12 - which is the version announced in our POM.

I can change this if we want LLE to be compatible with Jackson 2.11+ (just a matter of using an alternate constructor). If we do so, I would rather configure our pom the other way around and declare a dependency against Jackson 2.11 (the minimum supported version) and execute additional tests at build time against the more recent versions (and add them in the readme).
Our current strategy is indirectly causing a version bump of Jackson in people's projects - forcing them to downgrade if they want to stick to a specific version. If our pom declares the minimum required (or supported) version, users don't have other choice than upgrading. Upgrading is usually easier than downgrading ;-)

We already had this discussion in the past and you know my position: declare the minimum in the pom + execute additional tests against higher versions during build to ensure compatibility (+ list the compatible versions in the README).

I could not find a way to run the tests against both jackson 2.13.0 and 2.12.5 in a single maven command execution.

Maven does not support running unit tests multiple times with different class paths. Another option may be to "encapsulate" these compatibility tests in separate Maven modules (one per combination of dependencies). However this approach comes with its drawbacks: multiple modules to maintain, test cases must be defined in a separate module made available to the "compatibility test" modules, a non standard surefire configuration must be used, do not publish these modules, etc...
All this is probably not worth the effort... and your approach looks far easier to maintain !

@philsttr
Copy link
Collaborator Author

philsttr commented Nov 7, 2021

I'm fine with minimum jackson 2.12 for logstash-logback-encoder 7.0. Users can use an older version of logstash-logback-encoder if they still need jackson 2.11.

I'm still convinced that the best approach is to

  • allow dependabot to keep the version of jackson (and other dependencies) up-to-date,
  • add specific testing for the minimum jackson version supported,
  • mention the minimum supported version in the readme

@brenuart
Copy link
Collaborator

brenuart commented Nov 7, 2021

Ok. Let's keep going this way.

@philsttr philsttr merged commit 5b6cd61 into main Nov 7, 2021
@philsttr philsttr deleted the upgrade_jackson_and_run_tests_against_old_version branch November 7, 2021 21:54
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.

2 participants