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

test_root_register_add_member_senate.py: improvements #2209

Open
wants to merge 6 commits into
base: staging
Choose a base branch
from

Conversation

mvds00
Copy link

@mvds00 mvds00 commented Aug 7, 2024

Bug

The e2e test in test_root_register_add_member_senate.py searches for specific strings in specific lines (line numbers) of output. This does not make sense as the line number is not necessarily stable, nor relevant. When testing today the line numbers did not add up (did not investigate why exactly).

Description of the Change

As the ordering of items could be relevant, a function assert_sequence() is added that check whether expected items occur in the expected order. If lines would be injected between the expected lines, this doesn't affect the assertion. This is by design.

Also, the magic netuid number 1 is replaced by the variable netuid == 1.

Alternate Designs

It was considered to also assert that lines are adjacent. In practice the assert_sequence() will be most useful if injection of additional lines doesn't break the test. Such checks may be added later as a flag. It was considered to place assert_sequence() in some utils.py but it was decided to keep the change local to test_root_register_add_member_senate.py for now.

Possible Drawbacks

Not expected.

Verification Process

The new logic was verified by changing the expected string and observing the assertion being triggered.

Release Notes

N/A

@heeres heeres force-pushed the feature/mvds00/improve-test_root_register_add_member_senate branch from 4c16695 to 58c865a Compare August 7, 2024 19:40
gus-opentensor
gus-opentensor previously approved these changes Aug 8, 2024
@heeres heeres force-pushed the feature/mvds00/improve-test_root_register_add_member_senate branch 2 times, most recently from c863531 to 4f4359b Compare August 14, 2024 21:02
Copy link
Contributor

@thewhaleking thewhaleking left a comment

Choose a reason for hiding this comment

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

Other than improving the readability of the assert_sequence function, this is good to go.

Comment on lines 16 to 30
def assert_sequence(lines, sequence):
sequence_ptr = 0
for line in lines:
lineset = set(line.split())
seqset = sequence[sequence_ptr]
if lineset.intersection(seqset) == seqset:
sequence_ptr += 1
if sequence_ptr >= len(sequence):
# all items seen
break
assert sequence_ptr == len(
sequence
), f"Did not find sequence[{sequence_ptr}] = '{sequence[sequence_ptr]}' in output"


Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe make this a bit more readable:

def assert_sequence(lines: list[str], sequence: Collection[set[str]]):
    sequence_ptr = 0
    for line in lines:
        words_in_line = set(line.split())
        current_seq_set = sequence[sequence_ptr]
        if current_seq_set.issubset(words_in_line):
            sequence_ptr += 1
            if sequence_ptr == len(sequence):
                break

    assert sequence_ptr == len(sequence), (
        f"Did not find sequence[{sequence_ptr}] = '{sequence[sequence_ptr]}' in output"
    )

Copy link
Author

Choose a reason for hiding this comment

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

agreed, pushed this

…e numbers, change magic constant 1 to netuid

This fixed a prior dependence on line numbers, but the solution is still
useful as it captures the order of responses rather than words occurring
somewhere in the output.
@heeres heeres force-pushed the feature/mvds00/improve-test_root_register_add_member_senate branch from 4f4359b to 4edaa35 Compare September 3, 2024 13:40
thewhaleking
thewhaleking previously approved these changes Sep 3, 2024
@mvds00 mvds00 dismissed thewhaleking’s stale review November 12, 2024 22:50

The merge-base changed after approval.

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.

4 participants