HabitRPG / habitica-zapier

7 stars 5 forks source link

tests are needed #7

Open Alys opened 4 years ago

Alys commented 4 years ago

Tests. Tests tests tests.

Anyone who wants to write one or more tests is welcome to. Don't feel that you have to do a huge amount at once. A PR with even one new or improved test will help!

Majed6 commented 4 years ago

I'm almost done with github actions and variable BASE_HABATICA_URI. This should allow us to use Habitica staging for this repo and local instances if they're prefer by the developer.

Do you believe that a live approach is suitable ? Or should we simply mock the API response? Should we do both with mocked for PRs and live with push?

The issue with using a live version is that Forked PRs have to set their own secrets. Which in my opinion not a big deal.

Alys commented 4 years ago

@Majed6 Sorry, I'm not too familiar with this yet.

What secrets need to be stored, and who can see them? For example, is it just the User ID and API Token of a test account? Is it only github repo owners who can see them?

Habitica staging uses the same database as production, so there's no benefit to using it instead of production from the point of view of storing/reading test data. Staging requires the use of a web server username and password which does need to be kept secret to only staff and mods, so that complicates the use of staging.

Being able to easily use a local server and database would be excellent of course.

Depending on the answers to all that, my current preference is to use a live approach since mocking introduces extra complexity and an additional point at which something could go wrong. I reserve the right to completely change my mind though. :)

Majed6 commented 4 years ago

BASE_HABITICA_URI, USER_ID, and API_KEY . BASE_HABITICA_URI is optional since if it not defined it will default to Habitica production.

These secrets are only visible and utilizable by repository and its owner. I quote from

https://help.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets

"For a user account repository, you must be the repository owner to create encrypted secrets. For an organization repository, you must have admin access to create encrypted secrets. If you are using the REST API to create secrets, anyone with write access to the repository can create secrets. For more information, see "GitHub Actions secrets API" in the GitHub Developer documentation.......With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository."

I thought the staging was using a separate DB. Anyhow since the staging uses basic auth https://Aladdin:OpenSesame@www.example.com is always an option for BASE_HABITICA_URI.

I agree having them configurable is always good.

Live data has it benefits. We could easily spot unexpected changes in the API or invalid parameters quickly. Aside from having to explain how to add secrets in Github I see no downside to this. This is well documented in https://help.github.com/en/actions/configuring-and-managing-workflows/creating-and-storing-encrypted-secrets .

If it ever becomes cumbersome we could always mock it . See this PR

paglias commented 4 years ago

So, I agree that mocking would become quite annoying (although API v3 has been extremely stable for some time now) but I don't think we want tests against the production API.

It will pollute our analytics and other stuff. Could we launch the Habitica docker image in the github action and use it?

Majed6 commented 4 years ago

Great suggestion. I'll into the implementation as soon as I have the time. Shouldn't be too difficult. This would be a good reference .

We wouldn't need the secrets too. We can treat it as we would treat a local instance. 😄 👍

Majed6 commented 4 years ago

After quite a bit of tinkering today, I managed to build the pipeline as neatly as I could. Feel free to comment on the setup at https://github.com/HabitRPG/habitica-zapier/pull/10

Majed6 commented 4 years ago

With https://github.com/HabitRPG/habitica-zapier/pull/10 merged, maybe we should make these tests a PR check as documented here . This will ensure that no PR is merged without passing the automated test.

paglias commented 4 years ago

I'd say that for now we can skip that setting, PRs still get a passing / failing status that we can check before merging