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

NH-34752 Add config file tests + small changes #139

Merged
merged 14 commits into from
Apr 27, 2023

Conversation

tammy-baylis-swi
Copy link
Contributor

This mainly adds config file support tests. Changes:

  1. Moves existing test_apm_config.py to its own subdir
  2. Adds new test fixtures/mocks for return of Python with-file-open, get_cnf_dict helper.
  3. Adds new test_apm_config_cnf_file.py to test different scenarios for config file reading, updating config using file (with or without env vars also specified).

This PR also includes some small code changes:

  1. Removes unused configs inst and inst_enabled that were copied from AO but won't be in SWO
  2. Refactors get_cnf_dict helper out of existing update_with_cnf_file for easier testing

Since code was changed, here is a test trace that looks ok.

Please let me know if any suggestions! 😺

@tammy-baylis-swi tammy-baylis-swi requested a review from a team April 26, 2023 23:59
})

# pylint: disable=unused-import
from .fixtures.env_vars import fixture_mock_env_vars

Check notice

Code scanning / CodeQL

Unused import

Import of 'fixture_mock_env_vars' is not used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dismissing because pytest is using them.

from solarwinds_apm import apm_config

# pylint: disable=unused-import
from .fixtures.cnf_dict import fixture_cnf_dict

Check notice

Code scanning / CodeQL

Unused import

Import of 'fixture_cnf_dict' is not used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dismissing because pytest is using them.

Comment on lines +14 to +17
from .fixtures.cnf_file import (
fixture_cnf_file,
fixture_cnf_file_invalid_json,
)

Check notice

Code scanning / CodeQL

Unused import

Import of 'fixture_cnf_file' is not used. Import of 'fixture_cnf_file_invalid_json' is not used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dismissing because pytest is using them.

fixture_cnf_file_invalid_json,
)
# pylint: disable=unused-import
from .fixtures.env_vars import fixture_mock_env_vars

Check notice

Code scanning / CodeQL

Unused import

Import of 'fixture_mock_env_vars' is not used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dismissing because pytest is using them.

@@ -594,7 +596,7 @@ def update_with_env_var(self) -> None:
available_envvs = set(self.__config.keys())
# TODO after alpha: is_lambda
for key in available_envvs:
if key in {"inst_enabled", "transaction", "inst"}:
if key == "transaction":

Choose a reason for hiding this comment

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

Is there a reason for having transaction in self.__config.keys(). I think removing it from there will nullify this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's there as part of one way to store transaction with a default and transaction.prepend_domain_name as a nested object if set by customer, but I'm not convinced this is a good reason!

Right now an accepted config file would be:

{
    "transaction": {
        "prependDomain": false
    },
    "transactionSettings": []
}

But it might make more sense to customer if it were:

{
    "prependDomain": false,
    "transactionSettings": []
}

I'll change this in a next PR.

@tammy-baylis-swi tammy-baylis-swi merged commit fb00849 into main Apr 27, 2023
@tammy-baylis-swi tammy-baylis-swi deleted the NH-34752-config-unit-tests branch April 27, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants