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

Relax HTML validation for Fortunes test #9505

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jonathanhefner
Copy link
Contributor

Some frameworks, such as Next.js, automatically inject <meta>, <link>, and <script> tags into pages rendered by their templating system. Since the Fortunes test is meant to exercise the templating system (as opposed to raw string concatenation), it should allow these tags.

Furthermore, React warns against nesting a <tr> directly inside a <table> ("<tr> cannot be a child of <table>") because browsers will automatically wrap the <tr> elements in a <tbody>, causing a mismatch with the virtual DOM. Therefore, the Fortunes test should allow optional <tbody> (and <thead>) tags.

This commit relaxes the HTML validation for the Fortunes test to allow these tags by simply ignoring them when building the comparison string.

Some frameworks, such as Next.js, automatically inject `<meta>`,
`<link>`, and `<script>` tags into pages rendered by their templating
system.  Since the Fortunes test is meant to exercise the templating
system (as opposed to raw string concatenation), it should allow these
tags.

Furthermore, React warns against nesting a `<tr>` directly inside a
`<table>` ("<tr> cannot be a child of <table>") because browsers will
automatically wrap the `<tr>` elements in a `<tbody>`, causing a
mismatch with the virtual DOM.  Therefore, the Fortunes test should
allow optional `<tbody>` (and `<thead>`) tags.

This commit relaxes the HTML validation for the Fortunes test to allow
these tags by simply ignoring them when building the comparison string.
@jonathanhefner jonathanhefner mentioned this pull request Jan 8, 2025
@joanhey
Copy link
Contributor

joanhey commented Jan 8, 2025

I understand you.
But who need to change? the framework than inject extra magic, or the test ?

If any dev need that output, why the framework add extra magic ?

PD: also for a fair bench, it's necessary that all fw send the same output. If one fw send more bytes, the bench never will be good for that fw.

EDIT: it's terrible for any fw, but in any case the test need to send a warning to any fw that send extra bytes in the response.

@jonathanhefner
Copy link
Contributor Author

@joanhey As I hinted at in my commit message, it is possible to send exact HTML in these frameworks, but it boils down to raw string concatenation, and isn't a realistic way of writing an app.

If TechEmpower benchmarks are intended to reflect realistic usage of their frameworks, then this test should not demand exact output. In fact, the test already accommodates alternative HTML for character escaping (also here and here).

If a framework sends unnecessary bytes, then it will be penalized in terms of performance, but there is no reason to exclude those (realistic!) results from the benchmark.

@msmith-techempower
Copy link
Member

I think I regret the rigid way we built the verifier for Fortunes back in the day.

If I were to start over (and I did... there's a Rust verifier sitting collecting dust while I'm focused on other projects), I would make it only enforce that the response include the required elements - matching <tr> and </tr> tags and each required-and-balanced <td> and </td> tags with their correct bodies and orders (since that is part of the requirements). We would make any additional bytes beyond what the verification rules enforce currently be a warning with a link to the minimal example.

That would alleviate the strictness you, and others, feel when trying to make their framework pass.

I'm going to think on this particular relaxing of the Fortunes rules as a stopgap solution, but I doubt that it will be included for the forthcoming Round-to-be-published.

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