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 missing test for kubernetes compat converters #43248

Merged

Conversation

yangyulely
Copy link
Contributor

This PR will add unit tests for kubernetes compat converters, and improve the coverage to 100% of backwards_compat_converters.py
image
related: #35442


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Oct 22, 2024
@eladkal eladkal requested a review from romsharon98 October 22, 2024 11:52
@yangyulely
Copy link
Contributor Author

@romsharon98 , can you review this? I will add more test if that's okay.

@romsharon98
Copy link
Contributor

The tests looks good.
One small thing, in my opinion, when you want to test a function that calls a function inside it you will want to mock the inner call and just test the call has been done like you expected (and not really call the inner function).
Let me know what you think.

@yangyulely
Copy link
Contributor Author

The tests looks good. One small thing, in my opinion, when you want to test a function that calls a function inside it you will want to mock the inner call and just test the call has been done like you expected (and not really call the inner function). Let me know what you think.

Yeah, couldn't agree more with you, as you can see, that's how I got there

@patch("airflow.providers.cncf.kubernetes.backcompat.backwards_compat_converters._convert_kube_model_object")
def test_convert_volume_mount_normal_value(mock_convert_kube_model_object):
    mock_convert_kube_model_object.return_value = k8s.V1VolumeMount(
        name="test_volume_mount", mount_path="/mnt/test"
    )

@romsharon98
Copy link
Contributor

romsharon98 commented Oct 30, 2024

The tests looks good. One small thing, in my opinion, when you want to test a function that calls a function inside it you will want to mock the inner call and just test the call has been done like you expected (and not really call the inner function). Let me know what you think.

Yeah, couldn't agree more with you, as you can see, that's how I got there

@patch("airflow.providers.cncf.kubernetes.backcompat.backwards_compat_converters._convert_kube_model_object")
def test_convert_volume_mount_normal_value(mock_convert_kube_model_object):
    mock_convert_kube_model_object.return_value = k8s.V1VolumeMount(
        name="test_volume_mount", mount_path="/mnt/test"
    )

Yes but I think we need to add that we call the Mock function correctly for example in this one we should add:

mock_convert_kube_model_object.assert_called_with(...)

to really verify the logic of the function

@yangyulely yangyulely marked this pull request as draft November 1, 2024 07:46
@yangyulely yangyulely marked this pull request as ready for review November 1, 2024 08:50
@yangyulely
Copy link
Contributor Author

Yes but I think we need to add that we call the Mock function correctly for example in this one we should add:

mock_convert_kube_model_object.assert_called_with(...)

to really verify the logic of the function

@romsharon98, I have updated for that, will appreciate it if you could review again.

Copy link
Contributor

@romsharon98 romsharon98 left a comment

Choose a reason for hiding this comment

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

Looks good!

@romsharon98 romsharon98 merged commit 2c67c9f into apache:main Nov 1, 2024
67 checks passed
@yangyulely yangyulely deleted the backwards_compat_converters_test branch November 1, 2024 10:16
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants