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

Test Statistics Handler #12565

Open
wants to merge 2 commits into
base: jetty-12.1.x
Choose a base branch
from
Open

Test Statistics Handler #12565

wants to merge 2 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 24, 2024

Removed webapp for deleted Stats servlet
Updated module name to at least test deployment

Removed webapp for deleted Stats servlet
Updated module name to at least test deployment
@gregw gregw requested review from sbordet and lorban November 24, 2024 21:57
@gregw
Copy link
Contributor Author

gregw commented Nov 24, 2024

@lorban currently we have not distribution test for the StatisticsHandler. This PR at least tests the module deploys, but doesn't test that it works. Perhaps with the unit test it is enough? Better than what is there now.

@gregw gregw marked this pull request as ready for review November 24, 2024 21:58
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Besides the leftover, use DistributionTests.testLimitHandlers() as an example to deploy demo-simple-webapp to restore the original testing.

assertThat(htmlContent, containsString("<em>memory</em>: "));
assertThat(htmlContent, containsString("</body>"));
assertThat(htmlContent, containsString("</html>"));
// TODO test the StatisticsHandler somehow
Copy link
Contributor

Choose a reason for hiding this comment

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

We could make a couple requests, there already is the one above that returns a 200, and add another one that returns a 404, set dumpBeforeStop to true, then stop the server and check that the 2xxResponses and 4xxResponses counters are part of the dump and have the expected values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, but would be good to see requests in progress.


distribution.installBaseResource("stats-webapp-" + env + "/index.html", "webapps/demo/index.html");
distribution.installBaseResource("stats-webapp-" + env + "/WEB-INF/web.xml", "webapps/demo/WEB-INF/web.xml");
// TODO add some actual content to it
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this TODO can go away, the StatisticsHandler is installed as the root of the handler tree and there already is a deployed webapp that can be queried to check the stats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently it is just an empty directory, there is not webapp deployed.

@gregw
Copy link
Contributor Author

gregw commented Nov 25, 2024

@lorban if you want to take over this one... please do.... otherwise I'll get to it later in the week.

@lorban
Copy link
Contributor

lorban commented Dec 4, 2024

@gregw I'm now taking this over.

@gregw
Copy link
Contributor Author

gregw commented Dec 17, 2024

@lorban status of this one? Are you doing your own branch so I can close this?

@lorban
Copy link
Contributor

lorban commented Dec 18, 2024

@gregw I haven't resumed working on this just yet as I fell into the #12635 rabbit hole.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants