Unit Test Cleanup #368

pankajastro · 2022-05-20T09:59:16Z

This is an attempt to improve the unit test case this PR will add the below value

We have to not write context` fixture in each test file
We can move mocked third-party API response that we want to test out of test case files
We can reuse the create_context method which is currently redundant at dozen of place
We can move the constant like conn_name, poll_interval and other repeated param out of the test file and reuse it
test cases name will be smaller that would be easy to read
It would be easy to read the case since now it will have a nice grouping which mapped to the actual class
It would be easy to add a new test case and improve/change the existing test case since it would be grouped in a class
Running of test would be easier using pytest I can run a group of tests as well as individual test

Since this PR have only changes for core providers so above points are currently applicable on core provider only

Closes: #294
Related to: #568

codecov · 2022-05-20T10:17:19Z

Codecov Report

Merging #368 (e0f4dd0) into main (03d0bdc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #368   +/-   ##
=======================================
  Coverage   98.27%   98.27%           
=======================================
  Files          72       72           
  Lines        3996     3996           
=======================================
  Hits         3927     3927           
  Misses         69       69

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03d0bdc...e0f4dd0. Read the comment docs.

bharanidharan14

Need to test parametrize in class based approach

pankajastro · 2022-06-16T15:32:06Z

Need to test parametrize in class based approach

This shouldn't be a problem, I have checked in the apache repo

kaxil · 2022-07-07T13:14:42Z

What's the status of this PR?

bharanidharan14 · 2022-07-07T13:42:06Z

Need to test parametrize in class based approach

This should be a problem, I have checked in the apache repo

We can't have parametrize based function ?

pankajastro · 2022-07-08T11:46:02Z

Need to test parametrize in class based approach

This should be a problem, I have checked in the apache repo

We can't have parametrize based function ?

It is doable, it's a typo in the last comment, updated it. https://github.com/apache/airflow/blob/main/tests/providers/amazon/aws/hooks/test_base_aws.py#L704

pankajastro · 2022-07-08T11:51:08Z

What's the status of this PR?

@kaxil This was one of my suggestions to improve unit test reusability and readability during our retro call. I came up with some improvements for one module core so that we can conclude that discussion.

bharanidharan14

Overall LGTM, only thing can you add more description what is done on the cleanup activity, so that it can be avoided in the upcoming PR

bharanidharan14 · 2022-07-12T05:50:23Z

@pankajastro can you please fix the Description

bharanidharan14 · 2022-07-14T06:03:23Z

@pankajastro can we merge this ? Is anything need to be done ?

pankajastro · 2022-07-15T16:08:14Z

@pankajastro can we merge this ? Is anything need to be done ?

I'm in favour of merging it but let me re-evaluate it and get back to you.

pankajastro · 2022-07-15T16:33:17Z

@pankajastro can we merge this ? Is anything need to be done ?

Updated the description please let me know how this looks

pankajastro · 2022-07-19T07:28:41Z

I want to merge this but before going ahead I would like to know what other people think about these changes. WDYT @phanikumv @kaxil @pankajkoti @bharanidharan14 @rajaths010494 ?

kaxil

I would like to hear opinions from others :)

pankajkoti

The test coverage is dropping by 0.95% . Do we need to worry about it?

bharanidharan14 · 2022-07-27T07:10:27Z

tests/core/sensors/test_external_task.py

-    assert len(execution_dates) == 1
+
+class TestExternalTaskSensorAsync:
+    TASK_ID = "external_task_sensor_check"


can we add this variables inside setup_class like class variables?

do you mean I should create a setup class and keep this variable in that class? Can you please elaborate a little bit?

Yes, you can do like this

class Test: def setup_class(self) -> None: self.xxxx: str = "" self.yyyyy: str = '' self.connection_string = ( )

or you can create def test_init() inside the class

these variables are just single lines and getting used in this class only so moving outside to a different class not sure how much value will add. regarding variables like conn which might get used in multiple test classes, I'm planning to keep them in util

pankajastro · 2022-07-27T09:07:33Z

The test coverage is dropping by 0.95% . Do we need to worry about it?

If you see the delta changes #368 (comment) the coverage has dropped for most of the files which are not related to this PR so I don't think it matters much we still have all tests old test here

pankajastro · 2022-08-02T12:21:41Z

Created a follow-up ticket #568

pankajastro linked an issue May 20, 2022 that may be closed by this pull request

[POC] Improve unit test quality #294

Closed

pankajastro changed the title ~~Unit test cleanup~~ Unit Test Cleanup May 20, 2022

pankajastro requested a review from kaxil May 20, 2022 10:22

bharanidharan14 reviewed May 25, 2022

View reviewed changes

kaxil changed the base branch from main to release-1.4 May 25, 2022 12:07

kaxil changed the base branch from release-1.4 to main May 25, 2022 12:07

pankajastro force-pushed the unit_test_cleanup branch from 6785488 to f3871a8 Compare June 16, 2022 15:28

pankajastro marked this pull request as ready for review June 16, 2022 15:48

pankajastro requested review from phanikumv, rajaths010494 and pankajkoti as code owners June 16, 2022 15:48

kaxil requested a review from bharanidharan14 July 7, 2022 13:14

bharanidharan14 reviewed Jul 8, 2022

View reviewed changes

bharanidharan14 self-requested a review July 12, 2022 05:49

bharanidharan14 approved these changes Jul 14, 2022

View reviewed changes

pankajastro force-pushed the unit_test_cleanup branch from f3871a8 to 84c7b86 Compare July 15, 2022 16:06

kaxil approved these changes Jul 23, 2022

View reviewed changes

pankajkoti reviewed Jul 27, 2022

View reviewed changes

bharanidharan14 reviewed Jul 27, 2022

View reviewed changes

pankajastro added 4 commits August 2, 2022 17:50

Update core module unit test to classe based test

985e5ff

more cleanup

3af2288

Fix test

259d093

add default value for dag_id and run_id

e0f4dd0

pankajastro force-pushed the unit_test_cleanup branch from 84c7b86 to e0f4dd0 Compare August 2, 2022 12:20

pankajastro merged commit 261f1f5 into main Aug 2, 2022

pankajastro deleted the unit_test_cleanup branch August 2, 2022 14:38

Unit Test Cleanup #368

Unit Test Cleanup #368

Uh oh!

Conversation

pankajastro commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bharanidharan14 left a comment

Choose a reason for hiding this comment

Uh oh!

pankajastro commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaxil commented Jul 7, 2022

Uh oh!

bharanidharan14 commented Jul 7, 2022

Uh oh!

pankajastro commented Jul 8, 2022

Uh oh!

pankajastro commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharanidharan14 left a comment

Choose a reason for hiding this comment

Uh oh!

bharanidharan14 commented Jul 12, 2022

Uh oh!

bharanidharan14 commented Jul 14, 2022

Uh oh!

pankajastro commented Jul 15, 2022

Uh oh!

pankajastro commented Jul 15, 2022

Uh oh!

pankajastro commented Jul 19, 2022

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

pankajkoti left a comment

Choose a reason for hiding this comment

Uh oh!

bharanidharan14 Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

pankajastro Jul 27, 2022

Choose a reason for hiding this comment

Uh oh!

bharanidharan14 Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharanidharan14 Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

pankajastro Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

pankajastro commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pankajastro commented Aug 2, 2022

Uh oh!

Uh oh!

pankajastro commented May 20, 2022 •

edited

Loading

codecov bot commented May 20, 2022 •

edited

Loading

pankajastro commented Jun 16, 2022 •

edited

Loading

pankajastro commented Jul 8, 2022 •

edited

Loading

bharanidharan14 Jul 27, 2022 •

edited

Loading

pankajastro commented Jul 27, 2022 •

edited

Loading