pr-harmony icon indicating copy to clipboard operation
pr-harmony copied to clipboard

Use a separate ExecutorService to fix #71.

Open bturner opened this issue 6 years ago • 3 comments

  • Updated AsyncProcessor to look at the Bitbucket Server verson and choose between using the shared ExecutorService (5.9+) or creating its own ExecutorService (previous versions)
    • Bitbucket Server 5.9 includes a fix which prevents add-ons using the shared BucketedExecutor from blocking hook callbacks
  • Removed some dead code in AsyncProcessor and PullRequestListener
  • Simplified Maven structure and reconfigured how integration and unit tests are distinguished
    • Moved unit tests from src/test/java/ut to src/test/java
    • Added a simple UserResourceTest which requests configuration for a repository which hasn't had any applied
    • This verifies that the PR Harmony plugin has been installed and enabled successfully, and that its REST resource is available
  • Simplified some of the processing in UserUtils to use the API more efficiently and avoid various IDEA inspection warnings

bturner avatar Mar 01 '18 04:03 bturner

I've added a single REST integration test, which effectively verifies that the plugin starts and enables without issue. That means my code changes are structurally correct. However, I don't really have a good setup for actually testing the plugin. I apologize for the hassle, but you'll probably want to verify these changes yourself before you merge.

bturner avatar Mar 01 '18 04:03 bturner

Please hold off on this for a moment; I'm fixing some bugs I introduced (and adding more integration and unit tests as penance for doing so).

bturner avatar Mar 01 '18 22:03 bturner

Okay, I've pushed all my updates. I've verified my changes against Bitbucket Server 4.8.6 and 5.8.0, the oldest and newest versions the add-on claims to support. All seems well. (However, my validation has been based on my tests, so some things I haven't changed, like pull request auto-merging, haven't been tested. It's likely you'll still want to do your own validation.)

Apologies for the size of this pull request. I've probably gone too far fixing up issues. To try and increase your confidence in accepting my changes, I've added some new unit tests as well as several integration tests. ConfigResource and UserResource now have full REST tests, and CommitBlockerHook has an integration test which configures blocking and then validates handling of actual pushes end-to-end.

If my changes are too broad, you're welcome to pick them apart and use what suits you. I've generally tried to stick with what I perceive to be your code style (two space indenting, etc), but I may not have gotten everything quite right.

There's still one fairly pervasive issue with how the plugin is written that I haven't really tried to fix, and that's the inconsistent use of usernames and user slugs. For example, your User object has a slug field, but what you're actually putting in that field is the user's username, not their slug. I fixed a username/slug mismatch in CommitBlockerHook (which really should be PushBlockerHook, since Bitbucket Server cannot block commits; those are made on developers' workstations and then pushed to Bitbucket Server, at which point it can reject them), but there are almost certainly more. In many systems a user's username and slug are identical, but that's not guaranteed. For example, e-mail addresses ([email protected]) are valid usernames, but the slug for such a user would replace the @ with an _ (user_example.com). You might try creating a user like that and then doing some testing.

bturner avatar Mar 08 '18 01:03 bturner