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

Minor flowetl tweaks #929

Merged
merged 28 commits into from Jun 13, 2019
Merged

Minor flowetl tweaks #929

merged 28 commits into from Jun 13, 2019

Conversation

maxalbert
Copy link
Contributor

Just a few tweaks from my trying out #858, mostly just renamings to make some things a little more obvious at a glance.

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

  • Renamed TAG to FLOWETL_TESTS_CONTAINERS_TAG
  • Renamed the tag fixture to container_tag
  • Added a structlog logger (which also fixes a minor issue with logging messages from tests not showing up before)
  • Fixed a duplicate test name which shadowed one of the tests (the second one test_find_files_non_default_filter).
  • Currently os.getcwd() is used to determine the mounts folder, which relies on the tests being run in the correct directory. I have introduced a fixture flowetl_mounts_dir to make things slightly more robust.
  • Switched to non-standard ports for running the tests. @greenape has pointed out that it would be better to not assign any explicit ports but I'll leave this for a separate PR.
  • Renamed fixture data_dir to postgres_data_dir_for_tests
  • Added a separate fixture to pull docker images (mainly so that we can print some logging messages and the tests don't appear to be hanging)
  • Added a fixture to ensure that the AIRFLOW_HOME and TESTING env vars are set when running the tests.
  • Renamed test_airflow_home_dir to airflow_home_dir_for_tests because the former sounds like it's a pytest test function.
  • Removed some code duplication in wait_for_completion

Copy link
Member

@greenape greenape left a comment

Choose a reason for hiding this comment

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

👍


@pytest.fixture(scope="session")
def flowetl_mounts_dir():
return os.path.abspath(os.path.join(here, "..", "..", "mounts"))
Copy link
Member

Choose a reason for hiding this comment

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

Just a note for the future that it would be preferable if this stuff used tmpdirs instead of relying on empty directories we've tricked git into keeping around.

@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #929 into master will decrease coverage by 1.15%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #929      +/-   ##
========================================
- Coverage   93.16%    92%   -1.16%     
========================================
  Files         130    130              
  Lines        6657   6657              
  Branches      696    696              
========================================
- Hits         6202   6125      -77     
- Misses        331    400      +69     
- Partials      124    132       +8
Flag Coverage Δ
#flowapi_unit_tests 77.9% <ø> (ø) ⬆️
#flowauth_unit_tests 94.86% <ø> (ø) ⬆️
#flowclient_unit_tests ?
#flowkit_jwt_generator_unit_tests 100% <ø> (ø) ⬆️
#flowmachine_unit_tests 91.13% <ø> (ø) ⬆️
#integration_tests 59.87% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
flowclient/flowclient/client.py 56.52% <0%> (-37.2%) ⬇️

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 9a74291...3c96e82. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 13, 2019

Codecov Report

Merging #929 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #929   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files         130      130           
  Lines        6657     6657           
  Branches      696      696           
=======================================
  Hits         6202     6202           
  Misses        331      331           
  Partials      124      124
Flag Coverage Δ
#flowapi_unit_tests 77.9% <ø> (ø) ⬆️
#flowauth_unit_tests 94.86% <ø> (ø) ⬆️
#flowclient_unit_tests 82.6% <ø> (ø) ⬆️
#flowkit_jwt_generator_unit_tests 100% <ø> (ø) ⬆️
#flowmachine_unit_tests 91.13% <ø> (ø) ⬆️
#integration_tests 59.87% <ø> (-0.04%) ⬇️

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 9a74291...3c96e82. Read the comment docs.

@maxalbert maxalbert added the ready-to-merge Label indicating a PR is OK to automerge label Jun 13, 2019
@maxalbert maxalbert merged commit 6ee09e5 into master Jun 13, 2019
@maxalbert maxalbert deleted the flowetl-config-MA branch June 13, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FlowETL ready-to-merge Label indicating a PR is OK to automerge refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants