Lombiq/UI-Testing-Toolbox

Don't include a file called .htmlvalidate.json by default (OSOE-844)

sarahelsaig opened this issue · 7 comments

sarahelsaig commented

Right now we include .htmlvalidate.json in the project csproj file. This means if you place your own .htmlvalidate.json file in a project that references the NuGet package, it won't be enough. You also have to remove the import from the package first. This is not intuitive and as far as I can see it's not documented either.

Instead, I suggest renaming it to something like default.htmlvalidate.json and including a HtmlValidationOptions out of the box that uses .htmlvalidate.json if the file exists and default.htmlvalidate.json if it's missing.

Also I suggest including "form-dup-name": "off" in the default file, because we we have to turn that rule off for every project that uses boolean fields or just stock ASP.NET Core checkbox tag helpers because it expands into stuff like this:

<input name="OrderPart.IsCorporation.Value" type="hidden" value="false"><input name="OrderPart.BillingAndShippingAddressesMatch.Value" type="hidden" value="false">

Also backport this into UITT.

Jira issue

Piedone commented

Is the linked configuration a suitable workaround for you now? I.e., is this only about making configuration more convenient, or do we lack some functionality?

form-dup-name fails on input fields with duplicate names. Why would that fail on the example code? The names there are unique.

sarahelsaig commented

Is the linked configuration a suitable workaround for you now?

Actually no. Now the Linux run is broken that previously worked. 🫠

form-dup-name fails on input fields with duplicate names. Why would that fail on the example code? The names there are unique.

Oof, I pasted without thinking. Anyway, here it is:
image
And here is the responsible code. (this is caused by Microsoft.AspNetCore.Mvc.TagHelpers.InputTagHelper)
I guess the purpose of the duplicate names to send the default false value if the actual checkbox is not checked. Normal HTML behavior is to not send anything in POST when the box is unchecked. I suppose the ASP.NET Core folks wanted to avoid the ambiguity between false and missing.

Piedone commented

Please work on this if it's blocking for you.

OK, indeed we can use "form-dup-name": "off" in the default config.

Piedone commented

Following up from OrchardCMS/OrchardCore.Commerce#434, also include here the following:

  • Document how to use extends in the json to extend the default file.
  • Also add a per-test timeout configuration, see here.
Piedone commented

Hmm, we can actually set the timeout of async tests in xUnit directly: https://www.reddit.com/r/csharp/comments/zlulro/did_you_know_you_could_set_a_timeout_in_your/ So, I don't think we need a custom config, unless we can do a timeout that excludes waiting for the setup operation (that may be started by another test). However, that might work with the latest xUnit only, what I've updated to under #355.

sarahelsaig commented

Unless I misunderstood this is opt-in, so you'd have to add the Timeout to every Fact individually. Having a required limit that you can configure globally or per-test is still worth it.

Piedone commented

Yeah.