UCS/CONFIG: Filter out not loaded config tables for unused vars message #8557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
gleon99
reviewed
Sep 29, 2022
src/ucs/config/parser.h
Outdated
| @@ -91,6 +91,7 @@ typedef struct ucs_config_global_list_entry { | |||
| ucs_config_field_t *table; /* array of configuration fields */ | |||
| size_t size; /* size of config structure */ | |||
| ucs_list_link_t list; /* entry in global list */ | |||
| int loaded; /* entry was loaded by parser */ | |||
There was a problem hiding this comment.
WDYT of making a flags member instead of dedicating a whole int to a single bit?
Then can be extended if need extra fields in the future
gleon99
reviewed
Oct 12, 2022
src/ucs/config/parser.h
Outdated
| /** | ||
| * Configuration table flags | ||
| */ | ||
| enum { |
src/ucs/config/parser.h
Outdated
|
|
||
| typedef struct ucs_config_global_list_entry { | ||
| const char *name; /* configuration table name */ | ||
| const char *prefix; /* configuration prefix */ | ||
| ucs_config_field_t *table; /* array of configuration fields */ | ||
| size_t size; /* size of config structure */ | ||
| ucs_list_link_t list; /* entry in global list */ | ||
| int loaded; /* entry was loaded by parser */ | ||
| uint8_t flags; /* config table flags */ |
gleon99
reviewed
Oct 13, 2022
src/ucs/config/parser.h
Outdated
| * Configuration table flags | ||
| */ | ||
| typedef enum { | ||
| /**< Table was loaded by the system */ |
There was a problem hiding this comment.
has been already loaded by the config parser
test/gtest/ucs/test_config.cc
Outdated
| status = ucs_config_parser_fill_opts(&opts, car_opts_table, | ||
| UCS_DEFAULT_ENV_PREFIX, NULL, 0); | ||
|
|
||
| static ucs_config_global_list_entry_t config; |
yosefe
reviewed
Oct 17, 2022
test/gtest/ucs/test_config.cc
Outdated
| status = ucs_config_parser_fill_opts(&opts, car_opts_table, | ||
| UCS_DEFAULT_ENV_PREFIX, NULL, 0); | ||
|
|
||
| ucs_config_global_list_entry_t config; |
yosefe
previously approved these changes
Oct 24, 2022
|
@shasson5 pls squash |
yosefe
previously approved these changes
Oct 26, 2022
|
@shasson5 pipeline failure is relevant? |
|
yes I think so, will try to reproduce it |
gleon99
reviewed
Oct 27, 2022
test/apps/test_fuzzy_match.py
Outdated
| @@ -40,12 +40,23 @@ def __init__(self, ucx_info, verbose): | |||
| self.ucx_info = ucx_info | |||
| if verbose: | |||
| logging.basicConfig(level=logging.DEBUG) | |||
|
|
|||
| def ib_available(self): | |||
test/apps/test_fuzzy_match.py
Outdated
| {'UCX_SOME_VAR' : [], 'UCX_MOFULE_D' : ['UCX_MODULE_DIR', 'UCX_MODULES'], 'UCX_SOME_VAR2' : [], 'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']}, | ||
| {'UCX_RC_VERBS_RX_MAX_BUF' : ['UCX_RC_VERBS_TX_MAX_BUFS', 'UCX_RC_VERBS_RX_MAX_BUFS', 'UCX_UD_VERBS_RX_MAX_BUFS']}, | ||
| {'UCX_RLS' : ['UCX_TLS']}] | ||
| expected_list = [{'results' : {'UCX_LOF_LEVEL' : ['UCX_LOG_LEVEL']}, 'ib_required': False}, |
There was a problem hiding this comment.
WDYT of the following refactoring (simplify).
- Rename
expected_listto something liketest_casesorfuzzy_matches(at least remove_list). - Revert to the "old" structure of the dicts in the list
- Split to 2 lists, e.g
test_cases,ib_test_cases. - Then can have:
def run(self, test_case, needs_ib=False):
...
for test_case in test_cases:
runner.run(test_case)
for test_case in ib_test_cases:
runner.run(test_case, needs_ib=True)
test/apps/test_fuzzy_match.py
Outdated
| def run(self, expected): | ||
| with Environment(expected.keys()): | ||
| if expected['ib_required'] and not self.ib_available(): | ||
| # Skip test if IB transport is required but not available. |
There was a problem hiding this comment.
you mean it's not needed at all? or it should just be shorter?
yosefe
reviewed
Oct 29, 2022
| runner.run(expected) | ||
| ib_test_cases = [{'UCX_RC_VERBS_RX_MAX_BUF' : ['UCX_RC_VERBS_TX_MAX_BUFS', 'UCX_RC_VERBS_RX_MAX_BUFS', 'UCX_UD_VERBS_RX_MAX_BUFS']}] | ||
|
|
||
| for test_case in test_cases: |
There was a problem hiding this comment.
maybe smth like this
if self.has_ib()
test_cases += ib_test_casesand remove "needs_ib" param
yosefe
approved these changes
Oct 30, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Filter out not loaded config tables for unused vars message
Why ?
Avoid displaying wrong env var suggestions