-
Notifications
You must be signed in to change notification settings - Fork 271
Move the test suite to GitHub Actions #363
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
Move the test suite to GitHub Actions #363
Conversation
|
@safwanrahman Would be great to get your review here. |
|
Sample GitHub Action run: https://github.com/saadmk11/django-elasticsearch-dsl/actions/runs/1064789205 |
.github/workflows/ci.yml
Outdated
| run: | | ||
| sudo swapoff -a | ||
| sudo sysctl -w vm.swappiness=1 | ||
| sudo sysctl -w fs.file-max=262144 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need swap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe 7GB ram would be enough to run the test suite!
https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is recommended by elastic:
You must also add the Configure sysctl limits step, otherwise Elasticsearch will not be able to boot.
Ref: https://github.com/elastic/elastic-github-actions/tree/master/elasticsearch#usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@safwanrahman I have removed this and it looks like everything is working as expected, not sure why it was recommended by elastic, maybe there was some edge case before as the README was last updated more than a year ago.
Output: https://github.com/saadmk11/django-elasticsearch-dsl/actions/runs/1119509026
| attr1 = "foo" | ||
|
|
||
| field = DEDField(attr='attr2') | ||
| field = DEDField(attr='attr2', required=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to add it to required ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test tries to assert VariableLookupError is raised. but VariableLookupError is not raised if required is not True.
This test also fails locally.
django-elasticsearch-dsl/django_elasticsearch_dsl/fields.py
Lines 71 to 75 in 759076b
| if self._required: | |
| raise VariableLookupError( | |
| "Failed lookup for key [{}] in " | |
| "{!r}".format(attr, instance) | |
| ) |
|
Looks very nice! Thanks @saadmk11 |
This Pull Request moves the test suite to GitHub Actions from travis-ci.
Note:
codecov and travis-ci were not running previously.