Extend Webhook Support & Add Tests (#10)

assistify / rcapps-gitlab

RocketChat App to connect to GitLab and GitLab-CI

Apache License 2.0

16 stars 5 forks source link

Extend Webhook Support & Add Tests (#10) #18

Closed j0nm1 closed 4 years ago

j0nm1 commented 4 years ago

added GitHub Action
added test suite
added support for issues, merge-requests, pipelines
fixed a bug where user_avatar: null would generate an error on push webhook

known bugs:

encoding issues with quotes (but this is a bug within rocket.chat) see here: https://github.com/RocketChat/Rocket.Chat/issues/17688

Thanks @dkdndes for the task to work on this project and for your support!

mrsimpson commented 4 years ago

@j0nm1 First of all: Thanks a ton for you contribution! It's very much appreciated! I could only have a quick glance at the changes, but it looks very well engineered (particularly the testing part). Since the diff is quite huge (functional as well as technical), it will take us some time to review. But for sure, we'll get it merged probably without further major-changes :+1: We'll keep you posted here.

jschirrmacher commented 4 years ago

Why did you create a separate package.json file in the /test folder instead of using the one in the project root?

mrsimpson commented 4 years ago

@j0nm1 with respect to the test files:

It great that you added tests and also how you did that! However, I do have some questions / suggestions for the lot of the test messages:

is it necessary to use the full-fledged messages for the tests? I preferred to have test files stripped down to what matters in the test, then the are easier to maintain and also to observe what’s their difference.
there’s a lot of common parts in the test files. What do you think of extracting this common part (the common part which is necessary) and construct the message from code (instead of just reading the file). This would make it even more readable
so you Need the test data to refer to an actual repository for the app to work? I’d prefer if this was mocked data and particularly not referenced your users name

j0nm1 commented 4 years ago

@jschirrmacher I didn't even think about that, hope that's better now.

@mrsimpson I will look into what I can do. Currently, you definitely don't need all of the lines in the JSON files and I agree with you that it would be good to reduce the amount. You don't need a GitLab Repository to use the tests, I just created one to get the webhook content - mocked data would work as well.

dkdndes commented 4 years ago

@mrsimpson I suggest that @j0nm1 separates tests in a parallel pull request, so we have a starting point for testing. At the moment, I don't see tests in the repository, and I assume it would be fine to merge the add-on branch and start with a test branch in parallel?

mrsimpson commented 4 years ago

That's absolutely fine! I was in fact more concerned for @j0nm1 exposing some information of his private identity on a public repository than I was concerned about code quality, since as you noted: Anything is better than nothing 😬 I'll just have to find the time and at least install and test it once, then this PR is good to go, imho.

mrsimpson commented 4 years ago

@j0nm1 imho, you don't have to remove it unless you don't want your data exposed. If you're fine with that: Just leave them in here and strip them down in a separate PR. It's definitely good to have some tests, even if they can be streamlined!

j0nm1 commented 4 years ago

@mrsimpson I don't have a problem with the data being public but I think it would be good to anonymize it nevertheless (at least with some basic find and replace). I will open a separate PR for the test suite when it's ready.

j0nm1 commented 4 years ago

I removed all of the JSON files from the test suite and refactored the tests.

jschirrmacher commented 4 years ago

I'm sorry that it took a little while to test it. Now I've tested the app and it works great! Thanks for your contribution!