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

Extend test for OSGi manifest attributes #2738

Merged
merged 3 commits into from
Sep 8, 2024

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Sep 7, 2024

Purpose

Perform more extensive tests on generated OSGi attributes in MANIFEST.MF

Description

The test is converted to a Maven Failsafe integration test (...IT.java) to run on the final JAR, and additionally performs more assertions on the attributes.

However, there are the following disadvantages:

  • Maybe this single ...IT.java is confusing, compared to the regular ...Test.java
  • Executing this test in the IDE is not possible
    However, the test tries to detect this and produce helpful assertion failures in that case.
  • The test must be run with mvn clean ... due to [bnd-maven-plugin] Running bnd-process without mvn clean causes different behavior bndtools/bnd#6221
    (on the other hand this is maybe an advantage since this would probably also fail the release when for some reason clean is not performed, and the MANIFEST.MF would contain unexpected / not-reproducible content)

Therefore, I would not mind if you reject this pull request if you think there are too many disadvantages with this or if you think the new test is too complex or too difficult to maintain.

Checklist

  • New code follows the Google Java Style Guide
    This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
    Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>33.2.1-jre</version>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This direct dependency also fixes a different issue where previously truth and guava-testlib pulled in Guava transitively, but apparently truth's Guava -android version was chosen by Maven.

So maybe that could have lead to issues for guava-testlib which expected the -jre version.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

This looks fine (based on my limited knowledge). Just one small suggestion.

@eamonnmcmanus
Copy link
Member

Thanks for doing this!

@eamonnmcmanus eamonnmcmanus merged commit 48889db into google:main Sep 8, 2024
11 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/gson-osgi-import branch September 9, 2024 20:10
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