Add tests for error page mappings

[srinjoyray]

Issue - 318

[code-review-assist]

Hi @srinjoyray and reviewers,

Gemini is currently reviewing this pull request and will post a full review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request adds tests for error page mappings. The intent is to improve the robustness and reliability of error handling within the App Engine Java Standard environment. The changes involve the creation of new test files (SendErrorTest.java) and several supporting files containing HTML pages for error responses (404, 500, and unhandled errors) and a servlet to trigger error conditions. The tests cover various scenarios and runtime environments (Jetty 9.4, EE8, EE10) with both HTTP and HTTPS modes. The appengine-web.xml and web.xml files in the testapps directory have been updated to configure the error page mappings for each environment.

All in all, this PR looks to be a solid addition to the test suite, enhancing the overall quality of the App Engine Java Standard environment. I'll provide a more detailed review shortly.

And now, a little haiku to lighten the mood:

Tests run, green light shows,
Errors caught, the code is strong,
Happy users smile.

Thanks,
Gemini

[code-review-assist]

This pull request adds tests for error page mappings. It's a good practice to have tests for this functionality to ensure that errors are handled gracefully and users see informative messages. The parameterized tests effectively cover different runtime environments and HTTP modes.

[ludoch]

Also errors in https://github.com/GoogleCloudPlatform/appengine-java-standard/actions/runs/12357462444/job/34485765126

[lachlan-roberts]

I think Gemini covered a lot of good points which need to be addressed.

Some additional points.

• You are missing a lot of license headers on various files.
• You have senderroree10.WEB-INF as a directory name which will not work.
• Some of the files end with a single space, which will prevent them from being recognized.

The directory tree should look something like this

├── java
│ └── com
│     └── google
│         └── apphosting
│             └── runtime
│                 └── jetty9
│                     └── senderrorapp
│                       ├── SendErrorServletEE10.java
│                       └── SendErrorServletEE8.java
│                    
└── resources
    └── com
        └── google
            └── apphosting
                └── runtime
                    └── jetty9
                        └── senderrorapp
                          ├── ee10
                          │ ├── 404.html
                          │ ├── 500.html
                          │ ├── hello.html
                          │ ├── unhandled-error.html
                          │ └── WEB-INF
                          │     ├── appengine-web.xml
                          │     └── web.xml
                          ├── ee8
                          │ ├── 404.html
                          │ ├── 500.html
                          │ ├── hello.html
                          │ ├── unhandled-error.html
                          │ └── WEB-INF
                          │     ├── appengine-web.xml
                          │     └── web.xml
                          └── jetty94
                              ├── 404.html
                              ├── 500.html
                              ├── hello.html
                              ├── unhandled-error.html
                              └── WEB-INF
                                  ├── appengine-web.xml
                                  └── web.xml

Then in SendErrorTest.java you can reference it in the start() method with

String app = "com/google/apphosting/runtime/jetty9/senderrorapp/" + environment;

[ludoch]

Let's see now the extra checks in critique.

[ludoch]

Still 1 review to resolve to trigger google processes.

[lachlan-roberts]

Looks good! Just a few minor changes suggested.

[maigovannon]

@lachlan-roberts @ludoch Curious: is this a combination which exists now? i.e. Java21 and Jetty9.4? I thought we default to using Jetty 12 (either EE8 or EE10) for Java21 customers:

appengine-java-standard/appengine_init/src/main/java/com/google/appengine/init/AppEngineWebXmlInitialParse.java

Lines 97 to 101 in 78bbabe

	if (!settingDoneInAppEngineWebXml && (runtimeId != null)) {
	switch (runtimeId) {
	case "java21": // Force default to EE10.
	System.clearProperty("appengine.use.EE8");
	System.setProperty("appengine.use.EE10", "true");

[lachlan-roberts]

@maigovannon this is a combination which exists currently, but they must specifically configure these flags to use Jetty 9.4 on the Java 21 runtime. I believe it defaults to EE10 if none are set.