RITlug / teleirc

Go implementation of a Telegram <=> IRC bridge for use with any IRC channel and Telegram group
https://docs.teleirc.com/
GNU General Public License v3.0
138 stars 46 forks source link

Fix Imgur Unit Tests #101

Closed Tjzabel closed 5 years ago

Tjzabel commented 5 years ago

Problem

Imgur unit tests fail

Why?

The TgImgurPhotoHandler class is expecting an imgur object in its constructor. Currently, the mockImgur object is being passed into this parameter in the tests. This causes an error when running yarn test stating:

TgPhotoHandlerTests
✔ TgPhotoHandler_DisabledTest
✔ TgPhotoHandler_EnabledNoCaptionTest
✔ TgPhotoHandler_EnabledCaptionTest
Error when uploading to imgur: undefined
✔ ImgurPhotoHandler_SuccessTest

Possible Solution

I believe this is happening because the mockImgur object does not require('imgur') and so therefore does not pull from its properties. We need to figure out the best way to create a test imgur object.

Secondary Issue

Looking at this line of code in TgPhotoHandlerTests.js:

let photoSmall = {
    file_id : 1,
    file_size : 1,
    url : "https://ritlug.com/test/1.png",
    imgurUrl : "https://imgur.com/small.png"
};

The url field has a broken link. When the RITlug website was redesigned, these test images did not transfer over to the new site. We have added new assets to be used in place of these broken test images.

Secondary Solution

This is an easy fix Simply replace the https://ritlug.com/test/#.png links with the raw.githubusercontent.com links from each of the asset images.

Tjzabel commented 5 years ago

Update

After some debugging, it is still unclear whether or not this issue is related to mockImgur or not.

The guilty test case is the ImgurPhotoHandler_SuccessTest, specfically the second test case working with a photo that does not contain an imgur link.

When running only the first imgur test method, the undefined imgur error does not appear. This should narrow down the error to just the second test case within these imgur tests.

Debugging is still an ongoing process.

xforever1313 commented 5 years ago

Unit tests are supposed to mock out imgur, so require('imgur') is not needed. If the test is failing it could be because MockImgur doesn't have a function that we need to mock that we need to add.

The URL at ritlug.com never existed. Because we mock out imgur, it never actually queried ritlug, and never actually uploaded anything to imgur. The purpose of those unit tests is if imgur were to return a successful JSON message, does the correct message get relayed to IRC. The other unit test is if imgur were to return a failure by throwing an exception, does the ImgurHandler handle it okay.

photoNotInImgur purposefully does not have an imgurUrl field so MockImgur's upload URL does the failure case.

If we want to actually want to query ritlug.com and imgur, it is no longer a unit test; it is an integration/acceptance test.

Tjzabel commented 5 years ago

@xforever1313 ah, thanks for the overview! After the update, I had realized Imgur was not required.

There must be some other method that has not yet been overridden.

Tjzabel commented 5 years ago

Update

So, after extensive debugging, I've come to the conclusion that there's nothing really to fix. The only issue I see, is that when calling getFileLink from the mockTgBot, it returns undefined when calling the photoNotInImgur object because this object does not exist as one of its files. I'm not sure this is the desired functionality. I think it's supposed to return an undefined error when "uploading" to imgur, not because it can't find the tgUrl.

@xforever1313 is this supposed to be the case? Or is it supposed to return an error because it doesn't have an imgur url?

Either way, both options have the same end result, with Error when uploading to imgur: undefined being printed to the console.