Add tests for error page mappings#324
Conversation
ghost
left a comment
There was a problem hiding this comment.
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
ghost
left a comment
There was a problem hiding this comment.
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.
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Outdated
Show resolved
Hide resolved
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Outdated
Show resolved
Hide resolved
...stapps/src/main/resources/com/google/apphosting/runtime/jetty9/senderroree10.WEB-INF/web.xml
Outdated
Show resolved
Hide resolved
...estapps/src/main/resources/com/google/apphosting/runtime/jetty9/senderroree8.WEB-INF/web.xml
Outdated
Show resolved
Hide resolved
...pps/src/main/resources/com/google/apphosting/runtime/jetty9/senderrorjetty94.WEB-INF/web.xml
Outdated
Show resolved
Hide resolved
runtime/testapps/src/main/java/com/google/apphosting/runtime/jetty9/senderrorapp/500.html
Outdated
Show resolved
Hide resolved
...pps/src/main/resources/com/google/apphosting/runtime/jetty9/senderrorjetty94.WEB-INF/web.xml
Outdated
Show resolved
Hide resolved
lachlan-roberts
left a comment
There was a problem hiding this comment.
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-INFas 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;
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Outdated
Show resolved
Hide resolved
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Show resolved
Hide resolved
...main/resources/com/google/apphosting/runtime/jetty9/senderroree10.WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...main/resources/com/google/apphosting/runtime/jetty9/senderroree10.WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...main/resources/com/google/apphosting/runtime/jetty9/senderroree10.WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...stapps/src/main/java/com/google/apphosting/runtime/jetty9/senderrorapp/SendErrorServlet.java
Outdated
Show resolved
Hide resolved
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Outdated
Show resolved
Hide resolved
...stapps/src/main/java/com/google/apphosting/runtime/jetty9/senderrorapp/SendErrorServlet.java
Show resolved
Hide resolved
...stapps/src/main/java/com/google/apphosting/runtime/jetty9/senderrorapp/unhandled-error.html
Outdated
Show resolved
Hide resolved
.../main/resources/com/google/apphosting/runtime/jetty9/senderroree8.WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...in/resources/com/google/apphosting/runtime/jetty9/senderrorapp/ee8/WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...in/resources/com/google/apphosting/runtime/jetty9/senderrorapp/ee8/WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
...testapps/src/main/resources/com/google/apphosting/runtime/jetty9/senderrorapp/ee8/hello.html
Outdated
Show resolved
Hide resolved
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Show resolved
Hide resolved
ludoch
left a comment
There was a problem hiding this comment.
Let's see now the extra checks in critique.
ludoch
left a comment
There was a problem hiding this comment.
Still 1 review to resolve to trigger google processes.
lachlan-roberts
left a comment
There was a problem hiding this comment.
Looks good! Just a few minor changes suggested.
runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SendErrorTest.java
Outdated
Show resolved
Hide resolved
...esources/com/google/apphosting/runtime/jetty9/senderrorapp/jetty94/WEB-INF/appengine-web.xml
Outdated
Show resolved
Hide resolved
| <runtime>java21</runtime> | ||
| <application>senderror</application> | ||
| <version>1</version> | ||
| <system-properties> | ||
| <property name="appengine.use.EE8" value="false"/> | ||
| <property name="appengine.use.EE10" value="false"/> |
There was a problem hiding this comment.
@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:
There was a problem hiding this comment.
@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.
Issue - 318