[AIRFLOW-5241] Make test class names consistent by starting with Test

[BasPH]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-5241
  • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
  • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
  • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

• Here are some details about my PR, including screenshots of any UI changes:

I believe there are many ways to improve the current test suite. To start, I suggest to make the test class naming consistent by always starting with "Test".

Some insights:

• There are 557 test classes (grep -r "class" . | grep "TestCase" | wc -l)
• 240 of those start with "Test"
• 277 of those end with "Test"
• Leaving 40 classes with some other name, e.g. CliTests, PluginsTestRBAC and DagGcpSystemTestCase

There is no convention AFAIK in unittest for test classes (only tests themselves -> start with "test"). I suggest to make all test classes start with "Test" because I know there is some work being done on moving to Pytest, which discovers test classes starting with "Test" (link).

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

This PR involved renaming all non Test* named test classes.

I had to make one other change in tests/utils/test_helpers.py which contained two classes TestHelpers and HelpersTest, both testing exactly the same things, even with identical test names. I merged the two classes and merged the tests with identical names.

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

• Passes flake8

[BasPH]

@ashb tagging you here because I believe you were doing some preliminary work for testing with Pytest.

[milton0825]

Can we also include linting on the test name?

[potiuk]

Exactly my thought @milton0825 . Just wonder what will be the best tool ? Regexp? Some ast parsing?

[BasPH]

Quick Google search learned me that nosetests has a --collect-only flag. We could do something with that. Nosetests does some sort of autodiscovery so might also be worthwhile to check out the nosetests code to search for a "unittest class discovery function".

@potiuk why was this PR closed?

[potiuk]

Sorry. Wrote that on the phone :) . Autocomplete in Polish works great when I type in English. It was all by mistake -> I clicked wrong button.

[feluelle]

For pre-commit hook: tests' file names we can use name-tests-test with args: ['--django']

[ashb]

LGTM

[ashb]

Change looks good, but is probably worth adding some sort of lint-check as discussed.

[Fokko]

LGTM

Would also be great to make the filenames consistent, looking at you here tests/core.py 😀

[BasPH]

LGTM

Would also be great to make the filenames consistent, looking at you here tests/core.py 😀

Will do in another PR

[ashb]

@BasPH when you come to rename tests/core.py, be aware that one or two other test files do from tests.core import ... (😱 )

[BasPH]

@potiuk @Fokko @ashb had a think about it. Pytest requires test class names to start with Test (http://doc.pytest.org/en/latest/goodpractices.html#test-discovery), so instead of adding some custom script checking test class names I suggest to leave it as-is for now and leave the class name checking to Pytest when we move there. WDYT?

[ashb]

Am I right in thinking that if the name is wrong then py-test will simply "ignore" the tests and not run it? That's not great, and we ran in to that problem in the past with the file name of nose tests, so I think a custom script might still be needed?

[BasPH]

Pytest docs regarding test classes:

• Collects test prefixed test functions or methods inside Test prefixed test classes (without an init method)
• Within Python modules, pytest also discovers tests using the standard unittest.TestCase subclassing technique

I indeed read that Pytest will ignore tests not prefixed with test or in classes starting with Test, but why would that be a bad thing? (if we must do so for reasons, we can configure the test discovery patterns: http://doc.pytest.org/en/latest/example/pythoncollection.html#changing-naming-conventions)

[ashb]

In the past we add tests but because of bad naming they never ran, which is almost worse than not adding tests.

[BasPH]

Okay, isn't that up to the reviewer to correct?

If we were to implement some name enforcement utility, we'd have to first fetch all class definitions from all tests file, check if they start with Test, and for the non-Test classes, determine if they should be tests or not? Reason I'm unsure yet is because there are some helper classes here and there e.g. DummyClass in tests/utils/test_decorators.py. We would need some tricky logic to determine ourselves if such classes are actually tests or not.

[ashb]

Okay, isn't that up to the reviewer to correct?

It is, but the less load we can place on the reviewers the better. If it's possible without lots of effort anyway.

Everything in tests/utils is badly named btw -- they are utils for tests, not tests themselves.

[BasPH]

Ok, some more examples:

• Call in tests/operators/test_python_operator.py
• StubClass in tests/contrib/operators/test_grpc_operator.py
• SomeModelView in tests/www/test_security.py

All utility classes for the tests. What rule would you apply to such class names to determine if they're test classes or not and should start with Test or not?

I'd say if there's an assert somewhere in a class, it's probably for testing. But it's not waterproof because sometimes there's no assert but e.g. self.assertEqual.

[ashb]

Probably a lot of work then, you are right.

[BasPH]

Okay. I rebased and resolved all conflicts the gcp package created. Good to merge?