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

documenting how to run integration tests locally #380

Merged
merged 2 commits into from
Apr 7, 2023
Merged

documenting how to run integration tests locally #380

merged 2 commits into from
Apr 7, 2023

Conversation

edublancas
Copy link

@edublancas edublancas commented Apr 6, 2023

Describe your changes

Updates developer guide to document how to run integration tests locally.

Issue number

Closes #309

Checklist before requesting a review


📚 Documentation preview 📚: https://jupysql--380.org.readthedocs.build/en/380/

@edublancas edublancas marked this pull request as ready for review April 6, 2023 04:13
Copy link

@yafimvo yafimvo left a comment

Choose a reason for hiding this comment

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

Added 2 notes, besides that lgtm.

Some notes on the execution itself:

After executing pytest src/test/integration on Windows 11, the execution got stuck at this point:

image

Can't stop it/abort it (need to restart the command prompt)

This is the docker status

image

doc/community/developer-guide.md Outdated Show resolved Hide resolved
doc/community/developer-guide.md Outdated Show resolved Hide resolved
Copy link

@tonykploomber tonykploomber left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@idomic idomic left a comment

Choose a reason for hiding this comment

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

Couldn't move beyond installation.

doc/community/developer-guide.md Show resolved Hide resolved
@idomic idomic merged commit 0158a2a into master Apr 7, 2023
@idomic idomic deleted the dev-doc branch April 7, 2023 18:58
@idomic
Copy link

idomic commented Apr 7, 2023

@edublancas nice job!

@edublancas edublancas requested review from alextongme and removed request for alextongme April 7, 2023 21:32
@alextongme
Copy link

The links in the checklist are all dead

Checklist before requesting a review
 Performed a self-review of my code
 Formatted my code with [pkgmt format](https://github.com/ploomber/contributing/blob/main/CONTRIBUTING.md#lintingformatting)
 Added [tests](https://github.com/ploomber/contributing/blob/main/CONTRIBUTING.md#testing) (when necessary).
 Added documentation in the [docstring](https://github.com/ploomber/contributing/blob/main/CONTRIBUTING.md#documenting-changes-and-new-features) and [changelog](https://github.com/ploomber/contributing/blob/main/CONTRIBUTING.md#changelog) (when needed)

@alextongme
Copy link

hey team, while trying to run the integration tests, i got multiple failures all saying that each container was not ready fast enough.

Container postgres not ready fast enough.

is there any other setup i need to perform on docker besides downloading docker desktop?

@edublancas
Copy link
Author

@alextongme: unfortunately, we're still improving the installation process to be more compatible with several OSs. I suggest using the CI to test your PRs

@alextongme
Copy link

ah gotcha, ok sounds good. im not sure i will have much feedback to give on this PR then since i cant walk through the installation

```sh
pytest src/tests/TEST_FILE_NAME.py
```

Choose a reason for hiding this comment

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

I think it is also helpful to include how to run a specific test in a file likeso:

pytest src/tests/test_magic.py::test_save_with_trailing_semicolon

the required Docker images):

```sh
pytest src/tests/integration

Choose a reason for hiding this comment

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

When I get to this step, I received the error:

_____________ ERROR collecting src/tests/integration/test_mssql.py _____________
ImportError while importing test module '/Users/alextong/Documents/coding/jupysql/src/tests/integration/test_mssql.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../opt/anaconda3/envs/jupysql/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
src/tests/integration/test_mssql.py:1: in <module>
    import pyodbc
E   ImportError: dlopen(/Users/alextong/opt/anaconda3/envs/jupysql/lib/python3.10/site-packages/pyodbc.cpython-310-darwin.so, 2): Library not loaded: /usr/local/opt/unixodbc/lib/libodbc.2.dylib
E     Referenced from: /Users/alextong/opt/anaconda3/envs/jupysql/lib/python3.10/site-packages/pyodbc.cpython-310-darwin.so
E     Reason: image not found

Running brew install unixodbc seems to have fixed it as per the instructions on https://pypi.org/project/pyodbc/. I think this step should be added in before line 180 for users on Macs/Unix.

@@ -19,10 +19,28 @@ myst:

# Developer guide

Before continuing, ensure you have a working [development environment.](https://ploomber-contributing.readthedocs.io/en/latest/contributing/setup.html)

Choose a reason for hiding this comment

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

I think that this entire developer-guide.md file should be renamed and moved into a "Testing" -> "JupySQL" section within the contributing portal

I also found it a bit difficult to navigate to the contribution docs from the main website, and would suggest just making the "Developer guide" link pictured below directly go to Contributing since that covers all development instructions for all of your projects

Choose a reason for hiding this comment

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

Screen Shot 2023-04-14 at 1 17 08 PM

each PR; however, you might need to run them locally.

```{note}
Setting up the development environment for running integration tests locally

Choose a reason for hiding this comment

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

Perhaps, consider adding another note here to push developers to rely on the CI to test PRs since integration tests are not yet supported on all OSes

Choose a reason for hiding this comment

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

It might also be helpful to include which OSes are supported

Comment on lines +187 to +202
To run all integration tests (the tests are pre-configured to start and shut down
the required Docker images):

```sh
pytest src/tests/integration
```

```{important}
If you're using **Windows**, the command above might get stuck. Send us a [message on Slack](https://ploomber.io/community) if it happens.
```

To run some of the tests:

```sh
pytest src/tests/integration/test_generic_db_operations.py::test_profile_query
```

Choose a reason for hiding this comment

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

For consistency with the unit testing section, I would recommend this:

Integration tests are pre-configured to start and shut down
the required Docker images.

To run all integration tests:

```sh
pytest src/tests/integration
```

```{important}
If you're using **Windows**, the command above might get stuck. Send us a [message on Slack](https://ploomber.io/community) if it happens.
```

To run a specific file:

```sh
pytest src/tests/integration/test_generic_db_operations.py
```

To run a specific test:

```sh
pytest src/tests/integration/test_generic_db_operations.py::test_profile_query
```


Unit tests are executed on each PR; however, you might need to run them locally.

To run all unit tests:

Choose a reason for hiding this comment

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

i believe that with my added tests on the #319 change, running all unit tests locally started to fail due to a Too many open files error. i was able to bypass it by temporarily increasing my ulimit

ulimit -n 2048

another environment issue specific only to my mac, but having instructions published here in case other users encounter it might be worth writing since 256 is the default for macs.

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

Successfully merging this pull request may close these issues.

document local integration testing
5 participants