-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Integration test & unit tests jobs separation #6649
Conversation
@xirdneh you can push improvement on this branch as use this as a base. I am checking how service containers going to work here for integration tests |
pypy tests were failing before this PR. |
Codecov Report
@@ Coverage Diff @@
## master #6649 +/- ##
==========================================
- Coverage 89.23% 89.08% -0.16%
==========================================
Files 138 138
Lines 16623 16623
Branches 2099 2099
==========================================
- Hits 14834 14808 -26
- Misses 1570 1597 +27
+ Partials 219 218 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
redis connection is throwing errors. the service container might need some adjsutment |
@@ -63,3 +64,71 @@ jobs: | |||
flags: unittests # optional | |||
fail_ci_if_error: true # optional (default = false) | |||
verbose: true # optional (default = false) | |||
|
|||
|
|||
Integration: |
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.
We need to set the integration tests to only run after the unit tests to save resources.
See syntax: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idneeds
@matusvalo please suggest modification too. |
Let me check CI in celery. It should be pretty straightforward to port kombu/py-amqp CI to celery also... |
Just freshened this on master. I think I'll spend a little time on it today to see if I can get the redis suite quite working as a first step and then the rabbitmq/rpc prior to merge |
33d5350
to
a080602
Compare
The suite starts running now, but we have a busted test which fails the run after a few retries. |
This pull request introduces 1 alert when merging 1dac185 into 025bad6 - view on LGTM.com new alerts:
|
@@ -117,7 +117,7 @@ def test_active(self, inspect): | |||
'exchange': '', | |||
'routing_key': 'celery', | |||
'priority': 0, | |||
'redelivered': False | |||
'redelivered': ANY, |
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'm not sure why these tests stopped passing. AFAICT the redelivered
seems to be set to None
implying that we "don't know" (or don't bother to default to False
) if a task has been redelivered or not. These tests /probably/ (??) don't care about that so changing this to an ANY
seems like it is okay.
I'd like for someone a bit more familiar with redelivery logic to think about this if possible. I just don't know what we should be expecting here and why it's not False
like it must have been in the past.
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.
redelivered
is set when the broker redelivers the message (at least with AMQP brokers).
I honestly don't have an idea what has changed. Do you mind debugging this?
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'll see if I can find some time later this week
This pull request introduces 1 alert and fixes 2 when merging bfab125 into 025bad6 - view on LGTM.com new alerts:
fixed alerts:
|
Not sure why that inspect test is failing. It passes on my box reliably. Leaving this diff for the moment, someone else can feel free to take a look at it in the meantime. |
This pull request introduces 1 alert when merging 8f8d402 into 025bad6 - view on LGTM.com new alerts:
|
python-version: ['3.7', ] #'3.8', '3.9', 'pypy3'] | ||
# XXX: Does each matrix variation get their own copy of the service | ||
# containers? Test cross-talk may be an issue... | ||
toxenv: ['redis', ] #'rabbitmq'] |
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 should probably actually be called backend
since it's only the tail of the toxenv name.
I rebased this PR. |
This pull request fixes 2 alerts when merging 762668e into d3e5df3 - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging a1ae742 into d3e5df3 - view on LGTM.com fixed alerts:
|
What is wrong with the test suite? |
This seems pretty bad 😬 |
was the rabbitmq tests passing? I cant remember, I can only remember redis was failing. I will be doing some experiment in another PR to see if rabbit services & tests are running. I goose step at a time, lets see |
No description provided.