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

Issue #2266 Rework Scanner and use it for Jetty Maven Plugin #4239

Merged

Conversation

janbartel
Copy link
Contributor

Initial bug report: #2266
Problem diagnosis with PathWatcher class here: #4221

It is easier to rework the Scanner class a little to use Paths than to fathom how to fix the PathWatcher class. As this is jetty-9.4.x I've kept the old Scanner api methods, but deprecated them in favour of new Path-based methods. Scanner can be further cleaned up when it is merged through to jetty-10.

@janbartel janbartel requested review from gregw and joakime October 23, 2019 04:32
@janbartel janbartel added this to the 9.4.x milestone Oct 23, 2019
@joakime
Copy link
Contributor

joakime commented Oct 24, 2019

I'll test these changes on Microsoft Windows and OSX tomorrow.

@joakime
Copy link
Contributor

joakime commented Oct 28, 2019

I'll rerun the OSX and Windows tests on this PR in the morning.
So far, the prior runs were problematic on OSX (lots of test failures)

OSX Failures:

org.opentest4j.AssertionFailedError: Contexts.size ==> expected: <1> but was: <2>
	at org.eclipse.jetty.deploy.providers.ScanningAppProviderRuntimeUpdatesTest.testAfterStartupThenRemoveContext(ScanningAppProviderRuntimeUpdatesTest.java:146)

org.opentest4j.AssertionFailedError: Contexts.size ==> expected: <1> but was: <2>
	at org.eclipse.jetty.deploy.providers.ScanningAppProviderRuntimeUpdatesTest.testAfterStartupThenUpdateContext(ScanningAppProviderRuntimeUpdatesTest.java:173)

org.opentest4j.AssertionFailedError: Contexts.size ==> expected: <1> but was: <2>
	at org.eclipse.jetty.deploy.providers.ScanningAppProviderRuntimeUpdatesTest.testAfterStartupContext(ScanningAppProviderRuntimeUpdatesTest.java:129)

org.opentest4j.AssertionFailedError: Contexts.size ==> expected: <1> but was: <2>
	at org.eclipse.jetty.deploy.providers.ScanningAppProviderStartupTest.testStartupContext(ScanningAppProviderStartupTest.java:68)

 org.eclipse.jetty.maven.its.jetty-cdi-run-forked.jetty-cdi-run-forked
The post-build script did not succeed. assert buildLog.text.contains( 'helloServlet')

org.eclipse.jetty.maven.its.jetty-run-forked-mojo-it.jetty-run-forked-mojo-it
The post-build script did not succeed. assert buildLog.text.contains( 'pingServlet ok')

@janbartel
Copy link
Contributor Author

@joakime please make sure you're running the most recent checkin - 99f15cd

That's the most recent fixes.

@janbartel
Copy link
Contributor Author

@gregw please re-review.

@janbartel janbartel requested a review from gregw November 5, 2019 22:04
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM.
OSX isn't complaining.
I have a dead Windows machine ATM, so I cannot test on Windows until I get it fixed or replaced.

…66-reinstate-scanner-for-plugin

Signed-off-by: Jan Bartel <[email protected]>
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.

3 participants