Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Community question: How to test external services? #1

Closed
nikoksr opened this issue Jan 26, 2021 · 25 comments
Closed

Community question: How to test external services? #1

nikoksr opened this issue Jan 26, 2021 · 25 comments
Labels
help wanted Extra attention is needed type/feature New feature or request type/question Further information is requested

Comments

@nikoksr
Copy link
Owner

nikoksr commented Jan 26, 2021

Hi, dear reader,

notify is currently untested and I'd love to change that but I'll be straight up honest with you, I don't know how you test a library that's so dependent on external services. Like, do you create test/pseudo accounts/bots on all supported platforms and use the real api-tokens while testing? Do you actually make calls to the external services to verify the lib is working or can this be solved by mocking?

Feedback and PRs would be really appreciated! Hope you enjoy the library and have a nice day! :)

@nikoksr nikoksr added type/feature New feature or request help wanted Extra attention is needed type/question Further information is requested labels Jan 26, 2021
@j-wil
Copy link

j-wil commented Jan 26, 2021

My recommendation would be to test the same way your modules test and actually call the service.

https://github.com/go-telegram-bot-api/telegram-bot-api/blob/master/bot_test.go
Essentially what you are doing in your application is no different than how, for example, go-telegram-bot-api is testing their own service.

func TestSendWithMessage(t *testing.T) {
	bot, _ := getBot(t)

	msg := tgbotapi.NewMessage(ChatID, "A test message from the test library in telegram-bot-api")
	msg.ParseMode = "markdown"
	_, err := bot.Send(msg)

	if err != nil {
		t.Error(err)
		t.Fail()
	}
}

It would be nice to be able to mock these services but because you are abstracted 1 layer away from the actual API it's hard to mock a module. Another paradigm would be to call the telegram (and other services) APIs directly then you could write mocks instead of going through a module. The downside to this though is that you have to stay on top of the APIs because if they change your tests won't fail.

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 27, 2021

@j-wil thank you so much for your feedback! This really helps.

So, as I sort of wrote in the opening message, we need authorization tokens for all our supported services and then basically test the services under real conditions, right? Real conditions meaning that we actually send messages to said services.

@j-wil
Copy link

j-wil commented Jan 27, 2021

@nikoksr I believe so

I've done some digging and I found an article outlining how to test slacks APIs by mocking them but honestly that feels very burdonsom given the number of services that will need to be integrated into this project.

I really like what you're starting here hope I can help in some way.

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 28, 2021

@j-wil super grateful for your efforts and i totally agree with you!

The solution from the article would be good for a single news service but since we don't know where the project is going and possibly dozens of services could be supported at some point, the effort would probably be too great. I think that tests need to be reliable and consistent but also easy to implement, so I don't think the solution in the article would scale well for this project.

I will try a few test implementations in the next few days. I can link you in the PR as soon as I have a satisfactory solution, if you like.

Thank you so much. Really appreciate it! Yes, I would also be happy if you could continue to help with the project. Your feedback is already very helpful.

@j-wil
Copy link

j-wil commented Jan 28, 2021

@nikoksr No problem throw me into any PRs you would like

@ghost
Copy link

ghost commented Jan 30, 2021

Hello, I'm really a noob in code but I'm very interested in learning... I do not even know how to make a program that can test with a key api discord (I know to find it thank god) but I do not know how to give it to the program ... if someone could help me on this and on a possible main.go, so that I can try, I would be really grateful!! (i rly don't whant take your time :( )

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 30, 2021

Hey @kinkinkinkinkin, appreciate your interest for the project but this is really the wrong place to ask this question. You should open a new Q&A discussion here and ask your question there again.

@renanbastos93
Copy link
Contributor

Hello, Can I help you?

I thinking so to create unit tests using gock to mock requests APIs as the telegram.
What do you think?

Maybe we can create some tasks:

  1. Unit Test Telegram
  2. Unit test slack
  3. ...

best regards,
see ya

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 31, 2021

Hey @renanbastos93,
I didn't know about gock, looks great and like a potential solution. Do you have any experience using it?

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 31, 2021

@j-wil I was rethinking our approach of using real api tokens and testing all services under 'real' conditions. The first service that made me rethink that approach was the one for Microsoft Teams (#9).

As far as i know, there is no free version and therefore our tests would always depend on a community member having access to MS Teams. I then read the tests of the library that we use internally to communicate with teams api and came across this.

Since you know about MS-Teams; could we adopt these tests for our project? And I would also like to know your opinion about gock.

@j-wil
Copy link

j-wil commented Jan 31, 2021

@nikoksr Hm.. Good thoughts it seems like testing is going to be tricky on this project let me take a day or two and research gock as well as some other methods of testing and report back.

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 31, 2021

@j-wil tyt!

@renanbastos93
Copy link
Contributor

Hey @renanbastos93,
I didn't know about gock, looks great and like a potential solution. Do you have any experience using it?

hello @nikoksr,

I have experience using it. I can be creating an example later to show here. What do you think?

@nikoksr
Copy link
Owner Author

nikoksr commented Jan 31, 2021

@renanbastos93 an example would be great!

@j-wil
Copy link

j-wil commented Feb 2, 2021

@nikoksr I'm learning some new things here so this is good!

I believe what we need to do is stub the module. Here is a good article I found with examples. The examples utilize HTTP but really we just need to return the expected predefined result or error from the module.

Gock, I believe is accomplishing the same thing but I may have a couple of extra steps. @renanbastos93 Given you have experience with gock I'm really interested in your thoughts.

Regardless I have a couple of suggestions:

  • We will need to make sure the external service is well tested and stable. We are essentially only testing the logic/code in notify, not the external service
  • We probably need to set standards for the external services and maybe a review process to vet services we integrate. This can become a maintenance nightmare if we don't do it well from the start.
  • We will need a good tracking process of our external services because upgrading them will be an ongoing effort.

@KrishanBhalla
Copy link
Contributor

Following on from @j-wil - this would be a working example of the slack test in a manner similar to that article.
Due to Send returning only an error, this kind of testing is less granular than perhaps one would like.

Telegram is somewhat harder to mock as on creation of a telegram bot, the api token is validated (i.e. they use a real token)

@renanbastos93 - It would be interesting to hear more about gock and how that compares.

package slack

import (
	"testing"

	"fmt"
	"io/ioutil"
	"net/http"
	"net/http/httptest"
	"net/url"
	"strings"

	"github.com/slack-go/slack"
)

type mockSlack struct {
	Server *httptest.Server
}

func mock() *mockSlack {
	mockServer := &mockSlack{Server: mockServer()}
	return mockServer
}

func mockServer() *httptest.Server {
	handler := http.NewServeMux()
	handler.HandleFunc("/chat.postMessage", handlePostMessage)

	return httptest.NewServer(handler)
}

func handlePostMessage(w http.ResponseWriter, r *http.Request) {
	body, _ := ioutil.ReadAll(r.Body)
	kvs := strings.Split(string(body), "&")
	// eg: channel=foo&mrkdwn=false&text=some+text&token=SCRET_TOKEN&unfurl_media=false
	msgMap := make(map[string]string)
	for _, s := range kvs {
		kv := strings.Split(s, "=")
		s, err := url.QueryUnescape(kv[1])
		if err != nil {
			msgMap[kv[0]] = kv[1]
		} else {
			msgMap[kv[0]] = s
		}
	}
	// ref: https://api.slack.com/methods/chat.postMessage
	var response = `{
		"ok": true,
		"channel": "%s",
		"ts": "0000",
		"message": {
			"text": "%s",
			"type": "message",
			"subtype": "bot_message",
			"ts": "0.0"
		}
	}`

	s := fmt.Sprintf(response, msgMap["channel"], msgMap["text"])
	w.Write([]byte(s))
}

func testingSlackService() (*mockSlack, *Slack) {

	mockServer := mock()
	service := New("token")
	slack.OptionAPIURL(mockServer.Server.URL + "/")(service.client)
	return mockServer, service
}

func TestSlackSendMessageWithoutReceiver(t *testing.T) {
	// arrange
	mock, service := testingSlackService()
	defer mock.Server.Close()
	// act
	err := service.Send("subject", "message")
	// assert
	if err != nil {
		t.Errorf("Failed to send message without receivers. Received: %s", err.Error())
	}

}

func TestSlackSendMessageWithReceivers(t *testing.T) {
	// arrange
	mock, service := testingSlackService()
	defer mock.Server.Close()
	service.AddReceivers("Channel1", "Channel2")
	// act
	err := service.Send("subject", "message")
	// assert
	if err != nil {
		t.Errorf("Failed to send message to channels. Received: %s", err.Error())
	}
}

@nikoksr
Copy link
Owner Author

nikoksr commented Feb 3, 2021

@j-wil a great find; very interesting article and yes, I'm learning new things here too, which makes the project all the more exciting.

Very good thought of you to suggest the review process of external services. I also think we should definitely add that from now on to avoid future problems. Having to retroactively trace bugs back to external services would be super annoying. I think this should be a separate issue here, as I would like to discuss what parameters we use to determine if we use an external service. I will link the new issue here asap (#24).

On your third and last point, I need a little more explanation on what exactly you mean. What exactly do you envision as a suitable tracking process? Do you mean that we need to track when there are critical updates for external services, for example? In #17 there is a config for Dependabot that at least informs us when there are critical updates.

@nikoksr
Copy link
Owner Author

nikoksr commented Feb 3, 2021

@KrishanBhalla thanks for the example implementation. Here we come full circle again - we are back to the discussion of using real auth tokens, which would be okay as a solution but what do we do if we then have to implement a new service that does not allow free authentication tokens? I definitely want us to use a consistent testing scheme and not use gock for some services and real auth tokens for others.

@j-wil
Copy link

j-wil commented Feb 3, 2021

@KrishanBhalla Really good work!

@nikoksr KrishanBhalla mocked the entire slack service so the token does not have to be valid. If I understand correctly using this methodology there is no way to mock the telegram service specifically due to the nature of the module we are using. Perhaps we need to switch it out for one that matches our objectives.

Expanding on my third point "We will need a good tracking process of our external services because upgrading them will be an ongoing effort."
Dependabot is perfect for staying on top of security upgrades I'm more concerned about a scenario of we have 100 services integrated and 3 of them are using deprecated APIs or projects that are no longer maintained.

@nikoksr
Copy link
Owner Author

nikoksr commented Feb 3, 2021

@j-wil my bad, I think I wasn't clear enough. What I wrote about using real auth tokens was referring to the said problem with Telegram service and not @KrishanBhalla 's mocking example. It really looks like we can mock most services and may have to make an exception for Telegram. Annoys the hell out of me, but it's still better than having no tests at all.

Okay, thanks for the clarification. I see now. I sadly don't know of any GitHub actions or similar CI services that may help with this. And even if there were, we would have to check not only if the repository is no longer maintained but also if the repository still conforms to the api standard of the overlying service; I guess that's what you were thinking about too.

@KrishanBhalla
Copy link
Contributor

@j-wil - just to clarify the following point:

...if I understand correctly using this methodology there is no way to mock the telegram service specifically due to the nature of the module we are using. Perhaps we need to switch it out for one that matches our objectives.

I believe we can still mock, but it's significantly more complicated. When initialising a bot in the telegram code, they call this "GetMe()" method. It may be possible to mock the underlying http request here, and others in the pipeline.

Ms-teams and Pushbullet seem comparatively easy, much like Slack.

@prashanthpai
Copy link
Contributor

Just sharing my personal experience/opinion here about unit testing. Here's how I'd go about it:

  1. Create a local/unexported interface that only contains methods of backend/service that will be called.
  2. Assign the newly created service client to an instance of this interface.
  3. Create mock implementation of the interface for testing. During testing, you can define the behaviour of mock and assert the expectations. I have used https://github.com/stretchr/testify#mock-package for mocking and also used https://github.com/vektra/mockery for generating mocks from interface definition. You can use a mocking library of your choice.

Taking slack as an example, this is what it can look like:

type slackClient interface {
        PostMessage(channelID string, options ...slack.MsgOption) (string, string, error)
}

// verify at compile time that the type implements the interface
// if slack API changes, this will fail at compile time (a good thing)
var _ slackClient = new(slack.Client)

type Slack struct {
	client     slackClient
        ...
}

func New(apiToken string) *Slack {
	client := slack.New(apiToken)

	return &Slack{
		client:     client,    // assigning to interface
		...
	}
}

One downside I see with this approach is that it's not easy to test the client creation itself i.e this line:

client := slack.New(apiToken)

The above case will not fail but some other backend might give an error on client creation. For example:

client, err := slack.New(apiToken)
if err != nil {
        // some action here, and very often just this:
        return nil, err
}

In my personal opinion, this shouldn't be a big deal. As for unit tests, you can directly create a mock and assign to to the struct member. There are some ways to restructure the code to handle this better at the expense of code clarity and simplicity. But again, you'd want to address this shortcoming only if you're aiming for 100% synthetic coverage, which may or may not add any real value.

Also, I'd second the recommendation that we shouldn't be making any external network calls or use any real API tokens in unit tests. Hope this helps.

@nikoksr
Copy link
Owner Author

nikoksr commented Feb 22, 2021

Okay, so in my opinion we should go the way @prashanthpai suggested and add the rest of the tests. I really don't want to make the code more complicated or anything like that just to get a slightly higher percentage of successful tests. Besides, in the future, if we think of a better solution for our tests, we can change our testing procedure, it won't affect the compatibility of the library.

For now, I prefer to have a good part of the project tested first, rather than spending much longer looking for the optimal solution but not having tested anything at all.

We will do the other tests using the already implemented examples for Plivo and WhatsApp. Please, if anyone starts working on the tests, be it all services at once or just one, let us know so we can avoid duplicate work. Of course, everyone out there, is welcome to contribute these tests.

Appreciate you all.

cc @j-wil @KrishanBhalla @prashanthpai @checkaayush

@stale
Copy link

stale bot commented Apr 23, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Apr 23, 2021
@stale stale bot closed this as completed Apr 30, 2021
@nikoksr nikoksr reopened this May 25, 2021
@nikoksr nikoksr removed the wontfix This will not be worked on label May 25, 2021
@stale
Copy link

stale bot commented Jul 24, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jul 24, 2021
@stale stale bot closed this as completed Jul 31, 2021
@nikoksr nikoksr removed the wontfix This will not be worked on label Aug 14, 2021
nikoksr added a commit that referenced this issue Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed type/feature New feature or request type/question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants