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

Fix demo-spec webapp failures #10178

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jul 31, 2023

Fix for FooInitializer to correct renamed class file reference.

This sometimes caused a failure to start the webapp as the classname wasn't found.
This also caused a NPE on AnnotationTest.

Boolean complete = (Boolean)config.getServletContext().getAttribute("org.example.AnnotationTest.complete");
out.println("<p><b>Result: " + (complete.booleanValue() ? "<span class=\"pass\">PASS" : "<span class=\"fail\">FAIL") + "</span></b></p>");

This also caused FAIL conditions on the FooInitializer.onStartup() classes tests. (Either the attribute wouldn't be there, or it contained too few classes)

@joakime joakime added Bug For general bugs on Jetty side Jetty 12 labels Jul 31, 2023
@joakime joakime requested a review from gregw July 31, 2023 16:02
@joakime joakime self-assigned this Jul 31, 2023
Comment on lines -86 to +87
context.setAttribute("org.example.Foo", new ArrayList<Class>(classes));
ServletRegistration.Dynamic reg = context.addServlet("AnnotationTest", "org.example.AnnotationTest");
context.setAttribute("org.example.Foo", new ArrayList<>(classes));
ServletRegistration.Dynamic reg = context.addServlet("AnnotationTest", "org.example.test.AnnotationTest");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the bug that caused all of the varying failures.

@gregw gregw requested a review from janbartel July 31, 2023 17:26
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Some further refinements could be done, but as this is test code, it's not super critical.

context.setAttribute("org.example.Foo", new ArrayList<Class>(classes));
ServletRegistration.Dynamic reg = context.addServlet("AnnotationTest", "org.example.AnnotationTest");
context.setAttribute("org.example.Foo", new ArrayList<>(classes));
ServletRegistration.Dynamic reg = context.addServlet("AnnotationTest", "org.example.test.AnnotationTest");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should refer to the class, rather than the name of the class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AnnotationTest class is in the jetty-ee10-demo-spec-webapp project.
The above line in FooInitializer is in the jetty-ee10-demo-container-initializer project (It cannot see the AnnotationTest class)

@joakime joakime merged commit 4fe2b04 into jetty-12.0.x Jul 31, 2023
try (FileSystem fs = FileSystems.newFileSystem(uri, env))
{
Path root = fs.getPath("/");
IO.copyDir(depPath.resolve("target/classes"), root);
Copy link
Member

@olamy olamy Aug 2, 2023

Choose a reason for hiding this comment

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

uhm this assume you are running full build
this should use dependency:unpack or dependency:copy but not rely on the availability of build directory (this is breaking incremental/cache/partial build support :( )

Copy link
Member

Choose a reason for hiding this comment

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

fixed in a branch with this change b37aacf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submit PR with that change.

@joakime joakime deleted the fix/12.0.x/spec-webapp-testing branch August 2, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants