Skip to content

DRY in env code, fix e2e tests#489

Merged
AsafMah merged 13 commits intomasterfrom
fix-env
Jul 30, 2023
Merged

DRY in env code, fix e2e tests#489
AsafMah merged 13 commits intomasterfrom
fix-env

Conversation

@AsafMah
Copy link
Collaborator

@AsafMah AsafMah commented Jul 25, 2023

Fixed

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023

Test Results

    4 files  ±    0      4 suites  ±0   11m 32s ⏱️ + 10m 50s
  35 tests  - 197    35 ✔️  - 193  0 💤  - 1  0  -   3 
140 runs   - 788  140 ✔️  - 772  0 💤  - 4  0  - 12 

Results for commit 1f2b00d. ± Comparison against base commit 0d157a6.

This pull request removes 232 and adds 35 tests. Note that renamed tests count towards both.
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_app_cert_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_app_key_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_async_lock
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_az_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_azure_identity_token_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_base_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_basic_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_callback_token_provider
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_callback_token_provider_with_async_method
azure-kusto-data.tests.aio.test_async_token_providers.TestTokenProvider ‑ test_cloud_mfa_off
…
azure-kusto-data.tests.e2e.TestE2E ‑ test_cloud_info
azure-kusto-data.tests.e2e.TestE2E ‑ test_cloud_info_404
azure-kusto-data.tests.e2e.TestE2E ‑ test_log_analytics_query
azure-kusto-data.tests.e2e.TestE2E ‑ test_log_analytics_query_async
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_client[301]
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_client[302]
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_client[307]
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_client[308]
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_cloud[301]
azure-kusto-data.tests.e2e.TestE2E ‑ test_no_redirects_fail_in_cloud[302]
…

♻️ This comment has been updated with latest results.

@AsafMah AsafMah requested a review from yogilad July 25, 2023 08:36
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #489 (1f2b00d) into master (0d157a6) will increase coverage by 0.20%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   70.77%   70.97%   +0.20%     
==========================================
  Files          41       42       +1     
  Lines        3466     3487      +21     
==========================================
+ Hits         2453     2475      +22     
+ Misses       1013     1012       -1     
Files Changed Coverage Δ
...re-kusto-data/azure/kusto/data/_token_providers.py 54.77% <50.00%> (+0.24%) ⬆️
azure-kusto-data/azure/kusto/data/env_utils.py 95.45% <95.45%> (ø)
...ure-kusto-data/azure/kusto/data/_cloud_settings.py 90.90% <100.00%> (ø)
...zure-kusto-data/azure/kusto/data/client_details.py 77.19% <100.00%> (+0.40%) ⬆️

app_key = os.environ.get("APP_KEY")
os.environ["AZURE_CLIENT_SECRET"] = app_key

app_id = get_app_id()
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind these can now be None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How? if they don't exist it throws
I don't think you can set an empty env var

Copy link
Contributor

Choose a reason for hiding this comment

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

Meaning these had defaults which you omitted from the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, these are defaults in regard to our specific E2E test cluster. Which is very weird, and will fail for anyone who doesn't have the creds for it.
It will continue to work on github because the env is set properly, and also for anyone who configured their e2e tests.

yogilad
yogilad previously approved these changes Jul 27, 2023
Copy link
Contributor

@yogilad yogilad left a comment

Choose a reason for hiding this comment

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

Approving with suggestion:
Make sure this runs locally and does not go too nuts if local env does not include said env vars (which was the role of the defaults).

app_key = os.environ.get("APP_KEY")
os.environ["AZURE_CLIENT_SECRET"] = app_key

app_id = get_app_id()
Copy link
Contributor

Choose a reason for hiding this comment

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

Meaning these had defaults which you omitted from the code

@AsafMah AsafMah merged commit 7834c87 into master Jul 30, 2023
@AsafMah AsafMah deleted the fix-env branch July 30, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants