A poll votes and comments appreciated Right now you write te

Question

A poll votes and comments appreciated Right now you write test timeouts like this ```python tests timeout=120 ``` Should the value instead be like this ``` 120s 10s 2m 1 5m ``` A for the status quo B for setting the time unit Update going to accept both styles to avoid breaking BUILD files

fast-nail-55400 · Answer

Could it accept a number and a string in the position

hundreds-father-404 · Answer

What do you mean

fast-nail-55400 · Answer

i e convert using the units if a string but treat as seconds if a number

fast-nail-55400 · Answer

so `timeout=120` and `timeout= 2m ` would both be valid

hundreds-father-404 · Answer

Yeah we could do that Not sure it s worth the ambiguity tho that we have to put in our help strings everywhere in seconds Optimize for reading BUILD files over writing them Btw this idea comes from looking at how Go handles timeouts

fast-nail-55400 · Answer

re putting in seconds everywhere isn t that already the only supported unit currently

hundreds-father-404 · Answer

Yes we only support seconds currently But we have to make that clear in our help message because you can t be 100 sure from looking at only this BUILD file what the unit is ```python tests timeout=5 ``` Probably 5 seconds but could be 5 minutes

hundreds-father-404 · Answer

The help message ```timeout type int | None default None A timeout in seconds used by each test file belonging to this target ```

curved-television-6568 · Answer

could be another field too `python tests timeout after= 2m30s ` wink

hundreds-father-404 · Answer

what s the benefit of another field like we have both mechanisms that seems like overkill

happy-kitchen-89482 · Answer

120s is preferable in general but I fear that changing the type now would be disruptive to users and with only limited benefit

happy-kitchen-89482 · Answer

Since the unit is fairly obviously seconds in practice

happy-kitchen-89482 · Answer

I am very loth to break existing BUILD files any more than we have to

happy-kitchen-89482 · Answer

And this seems low priority

hundreds-father-404 · Answer

gt changing the type now would be disruptive to users could be mitigated by `update build files` I more want to get this right for Go I ve been adding timeout support today By extension we would then fix Python and Shell to use the preferred approach

hundreds-father-404 · Answer

gt low priority only a priority in that I want to get Go right

fast-nail-55400 · Answer

supporting both number and string would ensure we don t break existing BUILD files

hundreds-father-404 · Answer

And we could make me happy if we didn t document the legacy approach but still accepted it Thoughts

fast-nail-55400 · Answer

But that would leave users confused if they looked at the help text to see if the existing syntax were still valid right

happy-kitchen-89482 · Answer

I m confused as to why this needs to change for Go We can convert our ints into whatever the Go cmd line expects

hundreds-father-404 · Answer

Indeed which is what I m doing I m saying I d like to get the UX right for our Go plugin before we commit to something suboptimal

happy-kitchen-89482 · Answer

If we change the field type from int to string we can still ingest `120` on the cmd line or in env vars and just assume seconds What happens with toml Can an unquoted `120` be read as a string type

happy-kitchen-89482 · Answer

Oh wait this is in a target nm

hundreds-father-404 · Answer

Yep So the `timeout` field uses Target API and we can teach it to be type `str | int | None` for `pants toml` what s relevant is ` pytest timeout default` which should also use this same affordance imo Just verified that I can change `type=str` and using an int in the TOML file still works

happy-kitchen-89482 · Answer

So if we can continue to ingest existing BUILD files and config files as is then I see no problem in also accepting strings

happy-kitchen-89482 · Answer

we can document that seconds is the default unit

happy-kitchen-89482 · Answer

No need to be coy about it

hundreds-father-404 · Answer

Great

happy-kitchen-89482 · Answer

My strong reluctance is to break existing BUILD files if we can ingest them without change then all is well

incalculable-yacht-75851 · Answer

I prefer proper units prefer fewest options prefer not to introduce new parameters This problem touches all of these and they are at odds Since I m very new to both pants and python I don t really have a strong opinion However is this change necessary What would happen if you didn t make this change

witty-crayon-22786 · Answer

fwiw bazel has test sizes rather than per test timeout values i think that i like per test precise values more but something to consider

hundreds-father-404 · Answer

gt Since I m very new to both pants and python I don t really have a strong opinion Thank you for sharing your feedback < incalculable yacht 75851> It s a particularly helpful perspective you don t have the curse of knowledge Agreed with those preferences gt However is this change necessary Not strictly What we have works only is suboptimal and I was hesitant to add timeouts to Go using a suboptimal implementation But I m going to put this change down for now to focus on higher priority things Will create a ticket to summarize the idea and discussion