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

Stop throwing IllegalStateException #699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tjwatson
Copy link
Contributor

The following methods can stop throwing IllegalStateExceptions

Bundle.uninstall
BundleContext.createFilter
BundleContext.getBundle
BundleContext.getProperty
BundleContext.removeServiceListener
BundleContext.removeBundleListener
BundleContext.removeFrameworkListener
BundleContext.ungetService
ServiceObjects.ungetService
ServiceRegistration.unregister

Fixes #172

Copy link

github-actions bot commented Nov 15, 2024

Test Results

  663 files  + 3    663 suites  +3   1h 17m 11s ⏱️ -53s
2 209 tests + 8  2 162 ✅ + 8   47 💤 ±0  0 ❌ ±0 
6 771 runs  +24  6 628 ✅ +24  143 💤 ±0  0 ❌ ±0 

Results for commit daa4169. ± Comparison against base commit 2815608.

This pull request removes 2 and adds 10 tests. Note that renamed tests count towards both.
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.ExceptionMessageTest ‑ testUninstallContextError
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.ExceptionMessageTest ‑ testUnregisterTwiceError
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testContextUngetService
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testCreateFilter
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testFrameworkListener
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testGetBundle
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testGetProperty
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testRemoveBundleListener
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testRemoveServiceListener
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testServiceObjectsUngetService
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testUninstall
AutomatedTests org.eclipse.osgi.tests.bundles.BundleTests org.eclipse.osgi.tests.bundles.IllegalStateExceptionTests ‑ testUnregister

♻️ This comment has been updated with latest results.

The following methods can stop throwing IllegalStateExceptions

Bundle.uninstall
BundleContext.createFilter
BundleContext.getBundle
BundleContext.getProperty
BundleContext.removeServiceListener
BundleContext.removeBundleListener
BundleContext.removeFrameworkListener
BundleContext.ungetService
ServiceObjects.ungetService
ServiceRegistration.unregister
@tjwatson
Copy link
Contributor Author

If I merge this it will cause a permanent hard failure in one of the Framework TCK tests. That is fixed in osgi/osgi#787 but we likely will not see a TCK release for some time.

@laeubi is there any way to ignore that failure somehow?

I do not plan to merge this until development of 2025-03 opens up.

@vogella
Copy link
Contributor

vogella commented Nov 19, 2024

@tjwatson Not sure how you run the TCK tests but you can configure Maven surfire to ignore certain tests:

<build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <version>3.0.0-M7</version>
            <configuration>
                <excludes>
                    <exclude>**/MyFailingTest.java</exclude>
                </excludes>
            </configuration>
        </plugin>
    </plugins>
</build>

See https://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html

@laeubi
Copy link
Member

laeubi commented Nov 19, 2024

One thin I can think about would be using the SNAPSHOT of the OSGi repo once it is fixed there.

@tjwatson
Copy link
Contributor Author

@tjwatson Not sure how you run the TCK tests but you can configure Maven surfire to ignore certain tests:

<build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-surefire-plugin</artifactId>
            <version>3.0.0-M7</version>
            <configuration>
                <excludes>
                    <exclude>**/MyFailingTest.java</exclude>
                </excludes>
            </configuration>
        </plugin>
    </plugins>
</build>

See https://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html

Thanks, I can do that locally, I was wanting to ignore it for the PR validation runs. I will see if something like this can be done.

@laeubi
Copy link
Member

laeubi commented Nov 19, 2024

The TCK is not using usual surefire but https://tycho.eclipseprojects.io/doc/latest/tycho-surefire-plugin/bnd-test-mojo.html

@tjwatson
Copy link
Contributor Author

The TCK is not using usual surefire but https://tycho.eclipseprojects.io/doc/latest/tycho-surefire-plugin/bnd-test-mojo.html

If I understand that correctly, it appears I can use <exclude/> to exclude complete test classes, but not single test methods? I would not want to exclude all the tests in https://github.com/osgi/osgi/blob/main/org.osgi.test.cases.framework/src/org/osgi/test/cases/framework/junit/service/ServiceRegistryTests.java

@laeubi
Copy link
Member

laeubi commented Nov 19, 2024

We "only" call the bnd-test here so if there is an option to skip one test you can do it here

equinox/pom.xml

Lines 116 to 129 in 2815608

<properties>
<org.osgi.test.cases.framework.div.tb12>abc</org.osgi.test.cases.framework.div.tb12>
<org.osgi.test.cases.framework.div.tb15>abc</org.osgi.test.cases.framework.div.tb15>
<org.osgi.test.cases.framework.div.tb16>xyz</org.osgi.test.cases.framework.div.tb16>
<org.osgi.framework.system.capabilities.extra>
osgi.ee; osgi.ee="testOSGiEE",
osgi.ee; osgi.ee="AA/BB",
osgi.ee; osgi.ee="CC-XX/DD-YY",
osgi.ee; osgi.ee="EE/FF-YY"; version:Version="2.0",
osgi.ee; osgi.ee="GG-XX/HH"; version:Version="1.0",
osgi.ee; osgi.ee="II-1.0/JJ-2.0",
osgi.ee; osgi.ee="div/tb7a"
</org.osgi.framework.system.capabilities.extra>
</properties>

as far as I know last time I checked it was not that fine grained but might have changed (or can be improved) at bnd-lib.

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.

OSGi core R9 will remove ISE on some methods for Bundle, BundleContext, ServiceObjects and ServiceRegistration
3 participants