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

tests are needed #7

Open
Alys opened this issue Feb 23, 2020 · 8 comments
Open

tests are needed #7

Alys opened this issue Feb 23, 2020 · 8 comments

Comments

@Alys
Copy link
Contributor

Alys commented Feb 23, 2020

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
Copy link
Contributor

Majed6 commented Feb 24, 2020

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
Copy link
Contributor Author

Alys commented Feb 25, 2020

@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
Copy link
Contributor

Majed6 commented Feb 25, 2020

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:[email protected] 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
Copy link
Contributor

paglias commented Feb 26, 2020

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
Copy link
Contributor

Majed6 commented Feb 27, 2020

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
Copy link
Contributor

Majed6 commented Mar 1, 2020

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 #10

@Majed6
Copy link
Contributor

Majed6 commented Mar 4, 2020

With #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
Copy link
Contributor

paglias commented Mar 7, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants