Problem with an automated review: Review was only sent when committing the patch, not before.
Closed this issue · 16 comments
Phabricator URL: https://phabricator.services.mozilla.com/D14604
Problem: The comment from reviewbot was only seen after landing the patch, even though the patch was there for more than a day awaiting review.
Thanks for reporting this issue!
Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1514845 looks similar.
Happened again in https://phabricator.services.mozilla.com/D16711#452713
It happens all the time actually :(
@emilio sorry for the bad experience, I'm looking into this.
Thanks Andi! :)
I wonder if this couldn't be due to the fact that (IIRC) at the beginning revisions are "secure" for a bit and so we don't see them.
We are using search_diffs to retrieve the diffs:
We poll using search_diffs, storing the latest ID, then we poll again all diffs with ID > latest ID. Maybe, when we poll we don't get the "secure" revisions in the results, but we have the latest ID, so in the next poll we skip revisions that were "secure" and then turned to "normal".
This is just an idea, we have to check :)
(IIRC) at the beginning revisions are "secure" for a bit
Yes, I did recall correctly: https://wiki.mozilla.org/Phabricator/FAQ#Can_I_use_Phabricator_for_patches_to_security_bugs.3F.
Why is this kind:enhancement rather than kind:bug? This seems a bad issue for people to be hit by.
This just happened (again) in https://phabricator.services.mozilla.com/D25101 ... it's pretty annoying, tbh.
I'm working on this issue through several means:
- The last part of a serie of patches to improve stability & performance is under review
- We'll soon be able to use build plans in Phabricator to report when a code review task ran... or failed.
- Last but not least, code review analysis should run on Try pretty soon, thus avoiding a lot of depencies issues we are facing now.
We have an 81% success rate over the last week according to our dashboard
Again, i'm sorry your code did not get reviewed in time. Things should get better.
@jfkthame Your initial code review failed due to a mercurial clone taking too long in the bot.
This kind of issue will be fixed for our next release as it's adressed by the serie of patches i mentionnde.
Dumb question: why can't we make sure the bot also runs on revisions that switch from hidden to published states, if the revision itself is still open at that point?
Dumb question: why can't we make sure the bot also runs on revisions that switch from hidden to published states, if the revision itself is still open at that point?
The problem is that when we poll Phabricator we don't see the hidden revisions at all. This particular problem (which is rare compared to the failures due to clone timeouts) will be fixed with #1989.
* Last but not least, code review analysis should run on Try pretty soon, thus avoiding a lot of depencies issues we are facing now.
IME this doesn't help much for linting. At this point, I just expect linting to run against the patches I'm reviewing on phabricator, and if there are no comments I assume that's because the linter has run and there were no issues, and I don't look for issues as part of the review. I don't then spend 10 minutes trying to find trypushes for the patch. If submitted by new contributors, often there are linting issues, they don't have level 1 access so can't push to try, and then their patches get backed out.
@gijsk I meant code review tasks will run using Try infrastructure; it will be transparent for users (staff or new contributors).
Right now, we have to maintain all the deps needed to run ./mach lint|static-analysis|..., in the future we'll use the existing toolchains, thus avoiding issues
This should no happen again since the move to Try