URLValidator tests don't obviously test how long validation takes

[Chris Jerdonek]

I was looking at the validator tests and came across this comment ("Trailing junk does not take forever to reject"):
​https://github.com/django/django/blob/012f38f9594b35743e9ab231757b7b62db638323/tests/validators/tests.py#L258-L260

However, it's not obvious whether the test actually checks how long the validation takes, or what "forever" really means (e.g. is 1 second forever?). For example, I couldn't see any measurement of elapsed time in the tests.

It would be good if the comment elaborated on this and, if necessary, the test could check that validation doesn't take too long.

[Mariusz Felisiak]

However, it's not obvious whether the test actually checks how long the validation takes, or what "forever" really means (e.g. is 1 second forever?). For example, I couldn't see any measurement of elapsed time in the tests.

In this case "forever" means minutes (maybe hours). It's hard to estimate an upper bound, there are too many variables and we don't want to add a flaky test that fails on contributors computers.

... if necessary, the test could check that validation doesn't take too long.

Do you have a suggestion on how to handle this?

We can clarify a comment, but I don't think it's feasible to add a not error-prone test based on runtime.

[Chris Jerdonek]

Do you have a suggestion on how to handle this?

I had two changes in mind. The first was to update the comment with more explanation of what forever means. It can be as simple as adding what you said, e.g. "Trailing junk does not take forever to reject (minutes, maybe hours)."

The second was to add e.g. a context manager around each validation test case to check that the validation doesn't take longer than some amount of time. That would serve to check that it doesn't take forever. If the time is generous enough (e.g. 1 second?), there shouldn't be any risk of flakiness. (Unlike with other flaky tests, there is no I/O happening here.) If forever is truly minutes or hours, then even 60 seconds could work. But I think that might be unnecessarily generous.

If the second change isn't done (though I think it should), then I think the comment should be updated to clarify how this is being checked / what failure looks like, e.g. "Trailing junk does not take forever to reject (minutes, maybe hours). The test suite will hang indefinitely in this case." That was what originally confused me -- I couldn't see how this was being checked by looking at the code.