Skip to content

[AIRFLOW-7038] Cassandra sensors tests#7689

Closed
blcksrx wants to merge 1 commit intoapache:masterfrom
blcksrx:cassandra
Closed

[AIRFLOW-7038] Cassandra sensors tests#7689
blcksrx wants to merge 1 commit intoapache:masterfrom
blcksrx:cassandra

Conversation

@blcksrx
Copy link
Contributor

@blcksrx blcksrx commented Mar 10, 2020


Issue link: AIRFLOW-7038

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@blcksrx blcksrx force-pushed the cassandra branch 4 times, most recently from f55472b to 40d62df Compare March 11, 2020 15:43
@codecov-io
Copy link

Codecov Report

Merging #7689 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7689      +/-   ##
==========================================
- Coverage   86.98%   86.82%   -0.17%     
==========================================
  Files         904      904              
  Lines       43736    43736              
==========================================
- Hits        38045    37972      -73     
- Misses       5691     5764      +73
Impacted Files Coverage Δ
airflow/utils/dag_processing.py 88.33% <100%> (ø) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0%> (-45.08%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 69.69% <0%> (-25.26%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...flow/providers/apache/cassandra/hooks/cassandra.py 96.2% <0%> (+2.53%) ⬆️
airflow/utils/process_utils.py 86.04% <0%> (+12.79%) ⬆️
...rflow/providers/apache/cassandra/sensors/record.py 100% <0%> (+100%) ⬆️
...irflow/providers/apache/cassandra/sensors/table.py 100% <0%> (+100%) ⬆️

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 fcfc2aa...0a0d6df. Read the comment docs.

Comment on lines +42 to +50
hook = CassandraHook(cassandra_conn_id="cassandra_default")
session = hook.get_conn()
cqls = [
"DROP TABLE IF EXISTS s.t",
"CREATE TABLE s.t (pk1 text, pk2 text, c text, PRIMARY KEY (pk1, pk2))",
"INSERT INTO s.t (pk1, pk2, c) VALUES ('foo', 'bar', 'baz')",
]
for cql in cqls:
session.execute(cql)
Copy link
Member

Choose a reason for hiding this comment

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

This is not a pure unit test and it has side effects (the table is not dropped in tear down). I would suggest to mock connection with database (this will make the test faster) and add system tests (and example DAG). For example here's system test for PostgresToGCS:
https://github.com/apache/airflow/blob/master/tests/providers/google/cloud/operators/test_postgres_to_gcs_system.py

System tests are not run regularly but this hopefully will change soon as @potiuk is working on that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as the sensor uses the hook methods, what's your idea to mock the hook methods? For example, mocking record_exists in here:


    def poke(self, context: Dict[str, str]) -> bool:
        self.log.info('Sensor check existence of record: %s', self.keys)
        hook = CassandraHook(self.cassandra_conn_id)
        return hook.record_exists(self.table, self.keys)

Copy link
Member

Choose a reason for hiding this comment

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

I would mock CassandraHook and:

  • check if it was initialized with required parameter
  • the record_exists of initialized hook was called with expected parameters
  • check if the expected value was returned (mocking it)
  • make sure that all methods I mock are already covered by tests. In this case, I would check tests for CassandraHook

At least that is the approach we use for GCP operators / hooks. Once you cover the hook with tests you can mock the hook in operator tests. In my opinion, it's a nice way to separate and highlight what exactly is tested.

Of course, mocking only asserts that arguments are passed as expected and some methods are called. To check integrity we should use system test (that runs example DAG). It's something still discussed but is easy to do with SystemTest:
https://github.com/apache/airflow/blob/master/TESTING.rst#id24

@stale
Copy link

stale bot commented Apr 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 29, 2020
@stale stale bot closed this May 6, 2020
@blcksrx blcksrx deleted the cassandra branch November 20, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants