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

Enable tests to run on Windows #1242

Merged
merged 5 commits into from Apr 16, 2018

Conversation

Projects
None yet
3 participants
@c-w
Copy link
Contributor

commented Apr 16, 2018

I checked out the repository, followed the setup instructions and then ran the tests. I saw some errors so I thought that there is something wrong with my setup. After a bit of digging, I found out that my setup of the repository is fine but that some of the tests assume a *nix runtime or specific running external services.

In order to prevent this confusion for future developers, I thought to fix the tests on Windows such that after following all the setup steps, a new developer would get a clean test run and that any tests which can't run on the developer's machine are skipped instead of failed.

Specifically, this pull request implements the following:

  • 06b7f3b: Change documentation on how to run tests to use manage.py instead of py.test.
  • 14f4a98: Skip tests that rely on *nix features like fork and patch when running on Windows.
  • 8141fb6: Fix error due to default file encoding differences between Windows and *nix.
  • b2c45f3: Skip tests that rely on Redis unless the service is running.

The pull request also contains one unrelated fix that I noticed while working:

  • 623f48c: Remove an undefined variable.

For more detail on the changes above, please refer to the commit descriptions.

After applying this PR, following the setup and running the tests will lead to a clean test run on Windows:

Clean test run on Windows

Note that I do still get one error when running the tests (as seen in the screenshot above) but I also see that error when running the tests via Docker on the latest master (see screenshot below), so I'm assuming that this is a known issue and unrelated to my PR.

Test error on Docker

c-w added some commits Apr 16, 2018

Fix test run documentation on Windows
The pytest command referenced in the makefile fails on Windows with a
cryptic OSError: "The handle is invalid".

A full log of a test run can be found here:
https://github.com/Cal-CS-61A-Staff/ok/files/1916144/pytest-windows.log.txt

Running the tests via the management subcommand, on the other hand,
works without issues.

This commit changes the documentation to use the management subcommand
instead of the pytest command in the makefile. This might make it easier
for new contributors to get ramped up and verify their installations and
avoid confusion due to Windows and *nix differences.
Disable job and highlight tests on Windows
The rq module only supports *nix and as such the job tests fail on
Windows with an AttributeError: "module 'os' has no attribute 'fork'".

Similarly, the highlight tests depend on the *nix patch executable and
as such the tests currently fail on Windows with a FileNotFoundError.

This commit disables running of the job and highlight tests on Windows
to reduce these false positives which might make it easier for new
contributors to verify their installations.
Fix files tests on Windows
The files tests assume that the default encoding on the platform executing the
tests is UTF-8. However, on Windows CP1252 is the default encoding which leads
the file tests to fail case with a UnicodeDecodeError.

This commit changes the files tests to explicitly open the files as UTF-8 via
the io module which makes the tests pass on Windows.
Skip Redis tests if Redis is not available
Currently all tests that rely on Redis fail with a ConnectionError
unless the redis-server is run manually before starting the test runner.

This behavior could be confusing to some new contributors. As such, this
commit skips the tests that rely on Redis unless the service is actually
running.

Additionally, checking for Redis at test runtime lets us remove the
explicit ignoring of the test when running on Travis.

@colinschoen colinschoen self-requested a review Apr 16, 2018

@colinschoen

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Thanks for your work on this. Will review ASAP.

@colinschoen
Copy link
Member

left a comment

LGTM

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

Thanks for the super quick review, @colinschoen. Is there anything else I should be doing before you can merge this?

@colinschoen

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Nope, this looks great. CircleCI is borked because it is being run from an outside fork instead of a local branch on this repo (tracking at #1243). Will merge in a moment.

@colinschoen

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

@jathak Actually can you merge, we have master protected so I can't even manually merge from the CLI. I think owners should be able to override that.

@colinschoen

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Thanks @jathak. CircleCI is now passing.

@jathak jathak merged commit dc72cd6 into okpy:master Apr 16, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@c-w c-w deleted the c-w:cleanup/c-w/fix-test-run-on-windows branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.