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 helper classes for integration tests, refactor user managers and ankiserverctl.py #35

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cdpm
Copy link
Contributor

@cdpm cdpm commented Feb 5, 2016

Add test helper classes for creating and managing temporary files, working with anki collections and sqlite dbs. Add class for managing users so users can be added programmatically without using ankiserverctl.

…rking with anki collections and sqlite dbs. Add class for

managing users so users can be added programmatically without using ankiserverctl.
@dsnopek
Copy link
Owner

dsnopek commented Feb 15, 2016

This doesn't change any existing code to use these classes. Is that intentional (ie. these are meant to be completely seperate)? Or is that coming as a next step?

It's definitely easier to review if they are meant to be completely seperate, because it doesn't change anything, I can just merge it. :-) But if later code is going to be changed to use these classes, I'd prefer to review the addition of the new classes at the same time as the changes to the existing code.

Thanks!

@cdpm
Copy link
Contributor Author

cdpm commented Feb 21, 2016

I wrote some integration tests for SyncApp using WebTest that depend on the new test helpers but don't change existing code. I'll hopefully be able to review and commit them in this pull request in about a week.

…feature using WebTest's TestApp.

Add test helpers for creating, inspecting and manipulating instances of SyncApp and RestApp.
Add subclasses of Anki's RemoteServer and RemoteMediaServer for communicating with the wrapped SyncApp instance under test.
Add helpers for monkey patching Anki's MediaManager and DB for easier testing.
Add test assets directory.
@cdpm
Copy link
Contributor Author

cdpm commented May 7, 2016

Made a second commit a few weeks ago with the integration test code. In the future It might be useful to refactor ankiserverctl to use UserManager to avoid duplicate logic for managing users, but there are no direct changes to existing code right now.

@dsnopek
Copy link
Owner

dsnopek commented May 16, 2016

Thanks! I still think it'd be nice to have the changes to the existing code in the same PR, because it would be easier for me to review.

Christoph Mack added 2 commits May 27, 2016 21:33
…r and group all user managers in user_managers.py.
…erManager and use python3 compatible print calls.
@cdpm cdpm changed the title Add helper classes for integration tests. Add helper classes for integration tests, refactor user managers and ankiserverctl.py Jul 5, 2016
@cdpm
Copy link
Contributor Author

cdpm commented Jul 5, 2016

Committed changes to existing code.

script_dir = os.path.dirname(os.path.realpath(__file__))
ini_file_path = os.path.join(script_dir,
os.pardir,
"development.ini")
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't use development.ini - we should create a test.ini that's checked into the repo and is setup specifically for the tests. development.ini is intended for developers doing manually testing

@dsnopek
Copy link
Owner

dsnopek commented Aug 5, 2016

I've added a Travis CI configuration to the master branch. Please merge from master into your PR, and make any changes necessary to get the tests passing on Travis. Thanks!

@dsnopek
Copy link
Owner

dsnopek commented Aug 8, 2016

Thanks! The tests are failing because the 'sqldiff' command is unavailable, but the tests that don't depend on that appear to be working

tsudoko referenced this pull request in tsudoko/anki-sync-server Mar 5, 2017
To add a test for the new check, #35 would be needed to be merged
first.
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.

2 participants