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.

Sign up for GitHub

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

Jump to bottom

Fix slow server tests #763 #764 #765

Merged
merged 5 commits into from
Nov 24, 2020
Merged

Fix slow server tests #763 #764 #765

merged 5 commits into from
Nov 24, 2020

Conversation

ryosuketc
Copy link
Contributor

@ryosuketc ryosuketc commented Nov 19, 2020 •
edited
Loading

Optimize server tests. The PR also includes a small fix to support virtualenv.

Example usage and outputs:

 % tools/server_tests --skip_update_translations
--- Skipped updating translations

--- Running server tests
... (server tests continue) ...

 % tools/server_tests                                                                                       

--- Updating translations
... (update translations) ...

--- Running server tests
... (server tests continue) ...

@ryosuketc ryosuketc mentioned this pull request Nov 19, 2020
Closed
@gimite gimite self-requested a review November 20, 2020 01:41
gimite
gimite requested changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@gimite gimite left a comment

Choose a reason for hiding this comment

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

It is OK this time (especially since the PR is still small), but it is generally recommended to keep a PR small so that a single PR does a single thing (e.g., resolving a single issue) rather than combining multiple independent changes to a single PR.

tools/server_tests Outdated Show resolved Hide resolved
@ryosuketc
Copy link
Contributor Author

ryosuketc commented Nov 20, 2020 •
edited
Loading

It is OK this time (especially since the PR is still small), but it is generally recommended to keep a PR small so that a single PR does a single thing (e.g., resolving a single issue) rather than combining multiple independent changes to a single PR.

Thank you for the feedback.
Indeed I used different branches to make the changes but I thought they were too small for multiple PRs... I learn that a PR could never be too small. Appreciated.

@ryosuketc
Copy link
Contributor Author

Mentioned in #764 as well but it might be worth noting here in the PR itself too:

  • This code passes all tests except for:
    • Python 3 unit tests
    • (server tests) the image-related ones, which we can disregard for now as chatted offline with @gimite (with future migration to Python 3 etc in mind).

@gimite gimite merged commit 471331f into master Nov 24, 2020
@gimite gimite deleted the ryosuketc-fix-slow-servertests branch November 24, 2020 07:10
@gimite
Copy link
Contributor

gimite commented Nov 24, 2020

LGTM. Merged. Thanks!

ryosuketc added a commit that referenced this pull request Nov 25, 2020
@ryosuketc
Revert "Skip app/vendors from compilemessages #763" #765
4df9969 
This reverts commit 0af534c.

--ignore option for compilemessages is supported in greater than django 3.0.
https://docs.djangoproject.com/en/3.1/ref/django-admin/#cmdoption-compilemessages-ignore

makemessages support --ignore even on django 1.x
https://docs.djangoproject.com/en/3.1/ref/django-admin/#cmdoption-makemessages-ignore

Indeed, our django version is 1.11.29, which is the last version for Python 2.x
Django 1.11 is the last version to support Python 2.7. Support for Python 2.7 and Django 1.11 ends in 2020.
https://docs.djangoproject.com/en/3.1/faq/install/
@ryosuketc
Copy link
Contributor Author

ryosuketc commented Nov 26, 2020 •
edited
Loading

Send as a new PR.
#768

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gimite gimite gimite approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryosuketc @gimite