Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@mthrok
Copy link
Contributor

@mthrok mthrok commented Mar 1, 2021

Currently torchtext runs tests by pytest test. This is fine for development but one cannot run unit test on installed package in this manner because running test from the root directory adds the current directory as module search path and the checked out code directory will shadow the installed package. Similarly if you do (cd test && pytest .), this does not work either because pytest will figure out where the test module root starts and add the repository root to python module search path.

When running test on CI, it is more desirable to install the package and run tests because it can catch a bug related to packaging. Such as zip_safe=False issue. (this cannot be caught with the current configuration)

To resolve this issue, this PR

  1. introduces test/torchtext_unittest directory and moves all the test scripts and assets there.
  2. changes python setup.py develop to python setup.py install in CI job
  3. run test with (cd test && pytest torchtext_unittest) so that repository root is not in Python module search path.
  4. Change relative import (from ..common import ...) in test module to absolute import (from torchtext_unittest.common import ...)
  5. deletes pytest.ini, which hard-coded test path.

@mthrok mthrok changed the title Fix test run Fix the way tests are executed Mar 1, 2021
@mthrok mthrok marked this pull request as ready for review March 2, 2021 01:51
@mthrok mthrok force-pushed the fix-test-run branch 7 times, most recently from 41568bd to bfa9819 Compare March 6, 2021 04:09
mthrok added 3 commits March 10, 2021 05:05
Currently `torchtext` runs tests by `pytest test`. This is fine for development but one cannot run unit test on installed package in this manner because running test from the root adds the current directory as module search path and this will shadow the installed package. Similarly if you do `(cd test && pytest .)`, this does not work either because `pytest` will figure out where the test module root starts and add the repository root to python module search path.

When running test on CI, it is more desirable to install the package and run tests because it can catch a bug related to packaging. Such as `zip_safe=False` issue. (this cannot be caught with the current configuration)

To resolve this issue, this PR
1. introduces `test/torchtext_unittest` directory and moves all the test scripts and assets there.
2. changes `python setup.py develop` to `python setup.py install` in CI job
3. run test with `(cd test && pytest torchtext_unittest)` so that repository root is not in Python module search path.
4. Change relative import (`from ..common import ...`) in test module to absolute import (`from torchtext_unittest.common import ...`)
5. deletes `pytest.ini`, which hard-coded test path.
@facebook-github-bot
Copy link
Contributor

Hi @mthrok!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@mthrok mthrok closed this Sep 22, 2023
@mthrok mthrok deleted the fix-test-run branch September 22, 2023 02:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants