Codecov Report

Attention: Patch coverage is 84.09091% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.30%. Comparing base (3257487) to head (6f3a280).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main    #9371      +/-   ##
==========================================
+ Coverage   78.24%   78.30%   +0.06%     
==========================================
  Files         153      153              
  Lines       19040    19056      +16     
  Branches     2520     2523       +3     
==========================================
+ Hits        14898    14922      +24     
+ Misses       3856     3848       -8     
  Partials      286      286              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Please add tests to make sure the changes are behaving as expected 🙏

Thank you

Hi @Nusnus, thank you for looking at this PR. 🙏 I'm new here, and Python isn't my primary language. I may have rushed this. Let me make sure that I can run this locally and add the tests. As I look into this would you be able to take a look at the approach here and help confirm if the changes here are the right behavior?

Hi @Nusnus, I'm noticing that there are 2 smoke tests failing in specific environments that are succeeding in others here. Are you able to help me understand if those tests are flakey or not?

Hi @Nusnus, I'm noticing that there are 2 smoke tests failing in specific environments that are succeeding in others here. Are you able to help me understand if those tests are flakey or not?

Flaky.
Rerunning.

quick update: we think that the PR is ready for a first pass review. We won't be pushing any more commits at the moment.

We also think that code coverage lint is cached or something because we're seeing a check warning stating that "line #L150 was not covered by tests" in celery/backends/asynchronous.py#L150 even though we can confirm there are tests that fail if we decide to update that line to throw a random exception.

The only uncovered part is where this discussion thread is at. Happy to add tests if we reach a conclusion about the specific error and whether this is the approach that we'd like to take here.

Hey @Nusnus don't mean to be a bother, but is there anything we can do to help move this along? We're using this fork in production, but would love to upstream it (or get this into a state that you'd be willing to take on). Thanks!

Hey @Nusnus don't mean to be a bother, but is there anything we can do to help move this along? We're using this fork in production, but would love to upstream it (or get this into a state that you'd be willing to take on). Thanks!

I'll give it another look this week, let's see what I can do to help

@auvipy / @thedrow happy to take a look at smoke tests, but is there anything in particular you're looking for coverage on?

The only behavior change should be for users that are already on a non-happy path, the happy path shouldn't have changed. My gut says this may be tough to test without stubbing as it'd require killing redis from within the testing process (and then restarting it for other tests?). Let me know your thoughts, we'll still take a look today either way!

I stand corrected, does look like you have support for initing / killing containers in the smoke tests already -- we'll see what we can do :)

I stand corrected, does look like you have support for initing / killing containers in the smoke tests already -- we'll see what we can do :)

Check out the pytest-celery docs for the smoke tests: https://pytest-celery.readthedocs.io

CI Issues fixed, 100% passing now.

@auvipy I've been struggling to get the smoke tests to run locally, and don't want to spam you all/force CI to run broken tests while we figure it out. I've got a bit of time over the holiday break to make a second attempt, but I think we may be close to (or hitting) our limit in terms of being able to contribute effectively here :/

For what it's worth, I don't believe the codecov comment on this PR is accurate, I think it was cached based on earlier commits to the branch that didn't add tests.

@auvipy I've been struggling to get the smoke tests to run locally

What issues are you having?
Maybe I can help

@Nusnus how do you all recommend running the smoke tests on local? Was kind of hoping there'd be a make smoke or equivalent to build/boot docker with the right flags set to run the tests. I don't think anything here is insurmountable, we're just running up against the time we can allocate to this right now.

Where I left off was having permission issues in the container when invoking tox, need to figure out which dirs need to be mounted as writable and hope that's the last blocker to execution. Haven't spent a much time trying to get the tests to run outside docker, but it seems like some of the deps are missing prebuilt wheels for apple silicon and need extra system deps -- didn't really want to install anything into my env unless really necessary.