Skip to content

Conversation

CharliesPants
Copy link

Unit tests for representations.py and a couple integration tests. Names could probably be changed for tests for clarity. also some tests broke after an pull from main.

Copy link
Contributor

@justin-russell justin-russell left a comment

Choose a reason for hiding this comment

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

Looks good for the most part. I see that there are a few places where we need to do a bit more assertions. The integration tests that are ensuring things like spaces are being created look great!


# Broken with Update
"""
async def test_create_room_representation_success():
Copy link
Contributor

Choose a reason for hiding this comment

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

This still broken? What was wrong with it?

return_value=MagicMock(room_id="room_id"),
), patch(
"builtins.print"
) as mocked_print:
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 avoid mocking print. There should be some way to ensure that the function you called did something. In this case, it should have called room_create

# Broken on code update
# not important
"""
async def test_add_subspace_success_print():
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this test?


matrix_space = MatrixSpace()

# target.matrixcredentials_set.all.return_value = [] # Mocking empty matrix credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# target.matrixcredentials_set.all.return_value = [] # Mocking empty matrix credentials


create_subspace = MatrixSubSpace.create_representation_logs(instance, target)
assert len(create_subspace) == 2
print(create_subspace[0].method)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print(create_subspace[0].method)

assert "room_id" in result


def test_create_representation_logs(
Copy link
Contributor

Choose a reason for hiding this comment

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

This test looks almost complete.

MatrixSubSpace.create_representation_logs(instance, target) returns two items. We should verify what those two items are. They should be two representation logs, one to create a space for the instance, then another that adds the created space to the parent space.

assert create_subspace[0].instance == instance


async def test_create_subspace_reprsentation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear out the print statements in this test

assert target.metadata["room_id"] in room_ids


def test_create_subroom_reprsentation(
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in test_create_representation_logs, you should verify what the two items in the result are.


result = subspace.create_representation_logs(instance=target, target=primary_target)
# add an assert
assert result[0].instance == target
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 make sure to assert what we expect the .method to be, etc.

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