search

PalisadoesFoundation / talawa-api

API Backend for the Talawa Mobile App. Click on the link below to see our documentation
https://docs.talawa.io/
GNU General Public License v3.0
210 stars 721 forks source link

Testing enviornment for Redis Services #2037

Open Manik2708 opened 6 months ago

Manik2708 commented 6 months ago

Is your feature request related to a problem? Please describe.

Currently there are no tests written for src/services. These services are Cache Services and as project is moving forward, a testing environment for this has to be prepared.

Describe the solution you'd like

The solution according to me can be running a testing Redis server on a different port other than standard 6379. Mocking can bypass the tests making them pseudo, hence e2e (end-to-end) tests should be performed.

Approach to be followed (optional)

1) Firstly think of a possible way of dependency injection. Currently there is no Dependency Injection. A possible library for this can be tsyringe. If you have a better alternative to this, feel free to discuss. 2) Once library for DI is decided, make sure it is properly injected in all the resolvers and queries. 3) Now prepare a script for running testing redis instance if already not running. A package called wait-on can be used for this. 4) Write a test for any file in services so that it is easy for a reviewer to review this PR.

Additional context

Only thing which I want to say that if you have any other better approach, the please firstly discuss with maintainers. If possible firstly propose an approach in the issue and then start working.

Potential internship candidates Please read this if you are planning to apply for a Palisadoes Foundation internship https://github.com/PalisadoesFoundation/talawa/issues/359

Manik2708 commented 6 months ago

@palisadoes If this is relevant then can we talk on this?

varshith257 commented 6 months ago

@Manik2708 Once check @xoldyckk 's message in talawa-admin slack channel. The Redis recently moved away from open-source. maybe a case we have to move to another solution instead of Redis in the near future.

Manik2708 commented 6 months ago

Like when it is going to happen? Because till then I think we will be needy of tests. Without that code might break!

varshith257 commented 6 months ago

Might need guidance from @palisadoes for it

Manik2708 commented 6 months ago

@palisadoes xoldyckk This account is not available on GitHub. Please assist!

Cioppolo14 commented 6 months ago

@Manik2708 Check for discussions on redis in Slack, there have been some, maybe it will give you the guidance you need.

Manik2708 commented 6 months ago

@Manik2708 Check for discussions on redis in Slack, there have been some, maybe it will give you the guidance you need.

Actually it is not related to Redis, it is related to test Cache Services. Once current Cache Services are tested it will be easy to migrate to a new Caching Database. It will make it less buggy.

Manik2708 commented 6 months ago

I think we don't need to leave Redis. This PR solves the licence problem: https://github.com/redis/redis/pull/13169

Manik2708 commented 6 months ago

I think we don't need to leave Redis. This PR solves the licence problem: redis/redis#13169

This PR was closed just after this comment. I think we can wait for a while before reaching a migration decision.

xoldd commented 6 months ago

@Manik2708 Once check @xoldyckk 's message in talawa-admin slack channel. The Redis recently moved away from open-source. maybe a case we have to move to another solution instead of Redis in the near future.

It was just for informing Peter, I didn't say anything about needing to replace it. As far as I'm aware, the license doesn't prohibit usage of redis in the way it is used in talawa-api.

@palisadoes xoldyckk This account is not available on GitHub. Please assist!

Changed my username, thought the new one was more appropriate.

Manik2708 commented 6 months ago

@Manik2708 Once check @xoldyckk 's message in talawa-admin slack channel. The Redis recently moved away from open-source. maybe a case we have to move to another solution instead of Redis in the near future.

It was just for informing Peter, I didn't say anything about needing to replace it. As far as I'm aware, the license doesn't prohibit usage of redis in the way it is used in talawa-api.

@palisadoes xoldyckk This account is not available on GitHub. Please assist!

Changed my username, thought the new one was more appropriate.

Ok, thanks a lot for explanation. I think then we can talk on the approach to test Redis Services.

github-actions[bot] commented 5 months ago

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.

Manik2708 commented 5 months ago

@palisadoes Actually from the last week I was searching about DI in node js. I have written test including Redis in one of my personal project https://github.com/Manik2708/Hi_Server/blob/develop/test/Controllers/Confession/Services/send_confession.e2e-spec.ts, but problem is that this project is in Nestjs and it has an OOPs based structure. But unfortunately our Talawa-API is neither in Nest nor have OOPs structure. Also nodejs doesn't have a strong DI library.

I was thinking to test Redis Services in this way: 1) Auto injecting Redis client in the resolvers. When App will start, it will use redis-server running on main port (say 6379). 2) When running a test, a testing redis server will be used which would be running on a different port (say 6380).

I achieved this in Nestjs, but currently it is nearly impossible to execute this in Nodejs. So either we have ditch tests for redis services for now or we have to migrate from Nodejs+Graphql to Nestjs+Graphql.

Another advice from my side is to consider this migration important because as far I know, industries don't use Node js due to it's poor structure. As nodejs code increases, it becomes very difficult to maintain and test the code.

palisadoes commented 5 months ago

This is not an area of strength for me. If so the slack channel and / or create a GitHub discussion for this

xoldd commented 5 months ago

@Manik2708 you can have dependency injection using the graphql context. It is available to all resolvers as the third parameter of the resolver function.

github-actions[bot] commented 5 months ago

This issue did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please check if the develop branch has fixed it and report again or close the issue.