search

trilinos / Trilinos

Primary repository for the Trilinos Project
https://trilinos.org/
Other
1.21k stars 567 forks source link

autotester not testing new pull requests #2127

Closed ibaned closed 6 years ago

ibaned commented 6 years ago

@trilinos/framework @jwillenbring @allevin

Expectations

New pull requests should get tested once they are created, and "old" pull requests should be tested less to avoid a spam-like series of "passed" comments.

Current Behavior

Currently, the pull request autotester seems to work like a FIFO queue, meaning that new pull requests do not get attention until they are in the top 3 or 4 at the front of the queue (they start at the back). Also, until pull requests are merged, they remain at the front of the queue and continue to receive repeated (often unnecessary) testing.

Motivation and Context

I've put up a pull request but the autotester has not examined it in several days, rather its resources are focused on re-testing older PRs that have already passed autotesting several times and are waiting for action from the relevant developers. There is a significant trend in Trilinos of pull requests sitting idle for many days because their changes are non-trivial and package owners have limited availability to review them. Furthermore, even marking those older PRs as [WIP] does not seem to free up the autotester to look at newer ones.

Definition of Done

In short, the expensive operation of testing a commit should be used wisely to maximize throughput of pull requests.

Possible Solution

There are multiple possible approaches. A simple one would be to make the autotester work as a LIFO stack. It could also be event-based, with testing only occuring after certain events. Being created should be a key event, being approved by a developer could be another. If a developer has requested changes, there should be no testing until all changes are fully approved again. When resources are available to test something, PRs could be prioritized by most recent event.

bmpersc commented 6 years ago

@ibaned can you give an example of a PR that has been tested repeatedly? My admittedly not comprehensive search of pull requests didn't show any PRs that were tested multiple times. I did seem some that had many comments from the autotester reminding people that the PR was ready, but that does not re-run testing and takes almost no time to do so it is not what would be preventing newer PRs from getting tested.

I did see your PR that hasn't been tested yet, but I suspect that has to do with the same issue that is mentiond in PR #2126 which @allevin commented on.

ibaned commented 6 years ago

@bmpersc oh I see, I confused those comments with new testing. I suppose the reminder rate could be scaled back but I guess its not really using resources. I didn't know there were across-the-board issues with the autotester currently, that probably skewed the whole perspective. Lets wait to see how it goes once the current issues are resolved.

bmpersc commented 6 years ago

@ibaned I think that the issue was just discovered a couple hours ago so I don't think any of us were aware until this morning. Hopefully once the issue is fixed the backlog will be cleared quickly. Currently there are only 6 PRs that look like they need to be tested, ironically yours should be first with the current FIFO behavior. :)

allevin commented 6 years ago

@ibaned

Thanks for the feedback and suggestions. Let me try to address some of your comments.

• Be aware, at this moment 8:00am 1/5/2018 – The autotester is down due to a problem with unexpected data from Jenkins. I am working to address this issue. This is why you have seen no activity on your Pull Requests.

• The framework team needs to come up with a notification method to inform users when the autotester is down. I have written an issue regarding this in the autotester issues tracker.

• The autotester looks at all Pull Requests that are open and targeted to the develop branch. All Pull Requests marks with [WIP] or numbered less than 1700 are ignored. The Pull Requests are processed in numerical order (low to high). Once a Pull Request has been tested and is passing, there will be no additional testing performed unless there is a new commit to the branch.

• After testing has completed, The autotester will continue to post a message daily to notify the user that an action is required. i.e. Pull Request requires an inspection or needs to be merged. We do this to keep the user aware that there are pending actions on the PR to be taken. If no action is expected on the PR for a long while, we recommend the user adds [WIP] to the PR title.

• There was an issue with spam after testing completed. If a user made added comment in a PR, the autotester would repost its action required comment. I guess the autotester always wanted the last word :-) The autotester has been changed to eliminate this issue.

ibaned commented 6 years ago

Thanks @allevin ! That clears up a lot of things. I agree we should work to avoid stale PRs, so the current system may be fine as is. I'll close this because this new information explains a lot of the perceived behavior, if there is still something that seems wrong after the unexpected data thing is fixed then we can revisit it.