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

Add test instructions #365

Merged
merged 5 commits into from
Nov 18, 2024
Merged

Add test instructions #365

merged 5 commits into from
Nov 18, 2024

Conversation

javiermtorres
Copy link
Contributor

What's changing

Add more detailed instructions for testing the SDK and the backend.

How to test it

Check that the instructions match our current practice and the CI.

Additional notes for reviewers

N/A

I already...

  • added some tests for any new functionality
    • No auto tests needed
  • updated the documentation
  • checked if a (backend) DB migration step was required and included it if required
    • Doc changes only, no DB changes

@javiermtorres javiermtorres marked this pull request as ready for review November 14, 2024 10:52
Copy link
Contributor

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @javiermtorres docs++;!

I added some two suggestions, but I have two comments that maybe are more general than the improvements this PR brings.

  1. Should we look at moving some of the information in these README files to our operations guide?
  2. Do you think (in other places too) we should make it clear that when the user does SQLALCHEMY_DATABASE_URL=sqlite:///local.db ... to run tests, it creates (or uses if it already exists) a SQLite DB that isn't removed afterwards?

🤔

lumigator/python/mzai/sdk/README.md Outdated Show resolved Hide resolved
lumigator/python/mzai/backend/README.md Outdated Show resolved Hide resolved
lumigator/python/mzai/sdk/README.md Outdated Show resolved Hide resolved
@peteski22 peteski22 added the documentation Improvements or additions to documentation label Nov 14, 2024
@javiermtorres
Copy link
Contributor Author

Should we look at moving some of the information in these README files to our operations guide?

On the one hand, I do believe this belongs to https://github.com/mozilla-ai/lumigator/blob/main/docs/source/operations-guide/dev.md (also, maybe).
On the other hand, I think the dev instructions do not belong in an operations guide; it should imo include only info for an ops team to manage the service. (And also imo it's a veyr good thing that we're gathering info for an ops guide.)

@javiermtorres
Copy link
Contributor Author

Do you think (in other places too) we should make it clear that when the user does SQLALCHEMY_DATABASE_URL=sqlite:///local.db ... to run tests, it creates (or uses if it already exists) a SQLite DB that isn't removed afterwards?

It's a good point :-/ Maybe in a separate PR we should add a step in tests to remove this file, plus set this env var directly as a fixture (and maybe use sqlite:////tmp/local.db instead to minimize possible reuse of the file).

So yes, I'll add a line about it, at the very least.

javiermtorres and others added 3 commits November 14, 2024 18:26
Co-authored-by: Peter Wilson <[email protected]>
Signed-off-by: javiermtorres <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
Signed-off-by: javiermtorres <[email protected]>
@davidmanzanoai
Copy link

Should we look at moving some of the information in these README files to our operations guide?

On the one hand, I do believe this belongs to https://github.com/mozilla-ai/lumigator/blob/main/docs/source/operations-guide/dev.md (also, maybe). On the other hand, I think the dev instructions do not belong in an operations guide; it should imo include only info for an ops team to manage the service. (And also imo it's a veyr good thing that we're gathering info for an ops guide.)

We should separate both. Right now the focus should be on Dev side.

Copy link

@davidmanzanoai davidmanzanoai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the changes.

@javiermtorres
Copy link
Contributor Author

@peteski22 @davidmanzanoai @dpoulopoulos if it's ok with you, in another PR:

  • I'll move the dev.md guide to somewhere else (I'd propose get-started). I'd also like to do some light edits to clarify some issues, mainly the DB handling
  • I'll reference the READMEs from dev.md to avoid doc duplication, when talking about testing the backend and the SDK. @dpoulopoulos should we copy the READMEs content for doc building?

If you agree with the current status plus the scope of the proposed future PR, please approve this PR. Thanks in advance,

@javiermtorres javiermtorres merged commit c55de69 into main Nov 18, 2024
6 checks passed
@javiermtorres javiermtorres deleted the javiermtorres/test-instructions branch November 18, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants