See also T9853, which includes an extension to do this.

I'm generally not hugely excited about bringing this upstream. Although it's a relatively flexible tool, I think it's a fairly crude one and rarely/never the best tool for the job.

In this case, it would really be better to track deployment status separately, and actually show when things are deployed. We currently have no real concept of deployment, but may after T9530, perhaps within a year or so.

A different, equally crude workaround might be to rename "Resolved" to "Ready to Deploy" or similar using maniphest.statuses in Config, if the "resolved" language is a particular sticking point.

See also T9853, which includes an extension to do this.

I'll have a look at T9853 (sorry, didn't find that, when I searched... might have had "open" set), it looks close to what I had in mind at a first glance.

In this case, it would really be better to track deployment status separately, and actually show when things are deployed. We currently have no real concept of deployment, but may after T9530, perhaps within a year or so.

That might be a bit big for my use case, but if the general concept is there, I'm sure I could work something out through Conduit.

A different, equally crude workaround might be to rename "Resolved" to "Ready to Deploy" or similar using maniphest.statuses in Config, if the "resolved" language is a particular sticking point.

In my case it's not so much about the resolved status, it is more about the reminder, that it will take some time before the change will be effective. Basically telling the user: don't reopen this task just yet.

But maybe that could be a workaround for the time being, until T9530 is there: add an additional "close" state like "in deployment queue" which is set by the merge into the deployment branch and then have the deployment script actually "resolve" the issues through Conduit.

Ok, as far as I'm concerned I'm happy with what P1895 provides and this task can be closed. The more elegant solution seems to be part of T9530, making this task entirely superfluous.

Thanks @epriestley for the quick response!

I know about T9853, but if there's still a chance you might consider merging that upstream, there are a few folks here at Khan Academy who are excited by the feature. Here's one example:

If a differential review comes in that adds a @unittest.skip() line in one of our test files, one of my co-workers would like to be added as a reviewer. (We can do that now, obviously). He'd also like to automatically add a comment to the review: "What is your plan for re-enabling this test?" Right now he always has to a) figure out he was added to the review because of that rule, and then b) make the comment manually.

You said above that you think this feature is "rarely/never the best tool for the job," so if there's a better way to do something like that I'm sure my co-worker would be glad to hear it! But this seems like a pretty reasonable application of an "add comment" rule, to me.

Can you change the syntax to something like @unittest.skip("Reasoning behind this decision...")?

That is, I think the right tool for the job is either runtime enforcement or lint enforcement: make unittest.skip() with no reason fail (so the tests don't pass) or make lint raise a warning like "unittest.skip() must have a comment above it explaining the plan for re-enabling the test.".

In both cases, the enforcement tells the author what they need to do before they send the change for review, and points at exactly where in the code the problem is.

This use case is approximately "bad lint that happens too late in the pipeline".

Can you change the syntax to something like @unittest.skip("Reasoning behind this decision...")?

Well, that's what most people do (well, they do @unittest.skip("Reason why it breaks if you try to run it")). But what they typically don't do, and what my co-worker wants, is to describe a plan to fix the test so that it can run again.

I agree that this is really a lint check, but it's also a place that needs human judgment; we are not yet at the point where an automated linter could decide whether a reason-string describes a plan for fixing the test or not. I guess we could require the reason-string be some sort of structured text that a linter could look for.

Methinks that the Rube Goldberg solution for this would be to get unittest.skip to have another argument with a task number, and then validate with lint/runtime that it look like T1234. And then maybe grep the body of the ticket for the filename.