Verify the dict representation of a message has an isoformatted timestamp

[ReimarBauer]

Purpose of PR?:

Fixes #2356

[ReimarBauer]

that was missing for the isoformat change.

[ReimarBauer]

I think we should have both tests. That isoformat test was missing for the last change.

[matrss]

The linked issue describes a different thing that has already been fixed. The title of the PR does not match the changes (move vs add).

In my opinion we don't need a test for this get_message_dict anyway, what it does assert is trivially true and the function is being exercised by broader tests already. If we do unit tests on a too fine-grained level (e.g. for private helper functions) we will get ourselves into a situation where any architectural change will break tests (e.g. because the tested function is removed/renamed/its interface changed).

In my opinion, the most valuable tests are those that test the public interface of the code (in our case that is the msui Qt app as well as the mscolab and mswms APIs, where the latter two are best tested through the UI) and we should thrive to get full coverage of the code base through those high level tests. Anything more fine-grained has to be considered individually for its value.

[ReimarBauer]

The linked issue describes a different thing that has already been fixed. The title of the PR does not match the changes (move vs add).

changed issue and title

In my opinion we don't need a test for this get_message_dict anyway, what it does assert is trivially true and the function is being exercised by broader tests already. If we do unit tests on a too fine-grained level (e.g. for private helper functions) we will get ourselves into a situation where any architectural change will break tests (e.g. because the tested function is removed/renamed/its interface changed).

Where do we have a test, that we now require isoformatted time strings with a tz information?

In my opinion, the most valuable tests are those that test the public interface of the code (in our case that is the msui Qt app as well as the mscolab and mswms APIs, where the latter two are best tested through the UI) and we should thrive to get full coverage of the code base through those high level tests. Anything more fine-grained has to be considered individually for its value.

This tests a socket created chat message if it is conform to an iso formatted time string. How is that different to your last paragraph?

[matrss]

What I said was primarily directed at the MessageType check and the entirety of

MSS/tests/_test_mscolab/test_utils.py

Lines 66 to 71 in 531cb75

	def test_get_message_dict(self):
	message = Message(0, 0, "Moin")
	message.user = User(*self.userdata)
	message.created_at = datetime.datetime.now(tz=datetime.timezone.utc)
	result = get_message_dict(message)
	assert result["message_type"] == MessageType.TEXT

. I would just remove them.

Where do we have a test, that we now require isoformatted time strings with a tz information?

We don't, and I don't think we should have one unless using the iso format is part of our public interface. What we should have is a test that sends and receives messages from a client, exercising the serialization between client and server and making sure that messages reach each end as they should. We might need to have two or more instances of msui available in a test to properly do this. As it is, I consider the format of these timestamps as an implementation detail that is not important to check. Whenever we split MSS into multiple packages this might change, since then mscolab would have a public API and these timestamps would be part of it. In that case we should also think about versioning the API, though.

[ReimarBauer]

What I said was primarily directed at the MessageType check and the entirety of

MSS/tests/_test_mscolab/test_utils.py

Lines 66 to 71 in 531cb75

	def test_get_message_dict(self):
	message = Message(0, 0, "Moin")
	message.user = User(*self.userdata)
	message.created_at = datetime.datetime.now(tz=datetime.timezone.utc)
	result = get_message_dict(message)
	assert result["message_type"] == MessageType.TEXT

. I would just remove them.

I moved it because I recognized that I can't use it in a common event based usecase. So it was only with that Message class a check for the definition of the model. And by that a test for the get_message_dict. I think we should not remove it yet but maybe at 80% coverage organize tests differently.

Where do we have a test, that we now require isoformatted time strings with a tz information?

We don't, and I don't think we should have one unless using the iso format is part of our public interface. What we should have is a test that sends and receives messages from a client, exercising the serialization between client and server and making sure that messages reach each end as they should. We might need to have two or more instances of msui available in a test to properly do this. As it is, I consider the format of these timestamps as an implementation detail that is not important to check. Whenever we split MSS into multiple packages this might change, since then mscolab would have a public API and these timestamps would be part of it. In that case we should also think about versioning the API, though.

I think this different, when we haven't a test we need to communicate that often, while a test says "no, that is wrong".
It is too easy to miss that tzinfo or isoformat is set. Also on the refactoring I wished I had an idea how to add that test, the string which I missed would may be crashed it.

[matrss]

I think this different, when we haven't a test we need to communicate that often, while a test says "no, that is wrong".

Yes, what I am saying is just that we should make a conscious decision on what we test, and why, because overdoing it on the details is a net-negative. If we want to declare that all timestamps must be encoded with isoformat, then that is fine to add as a test; it then becomes part of the expected interface instead of remaining an implementation detail.

My stab at test_get_message_dict is meant to say that this test encodes that we must have a helper function called get_message_dict and that it must return a dictionary with a message_type key, while the value of that key must be MessageType.TEXT for the specific message we gave to it. That is hardly a useful tests, and it would just be a hindrance when refactoring that part of the code. Instead, the important thing to test is that a message sent from one client to another arrives unaltered, and if it uses this helper function on its way then we get coverage for it for free.

[matrss]

If we want to declare that all timestamps must be encoded with isoformat, then that is fine to add as a test; it then becomes part of the expected interface instead of remaining an implementation detail.

I am still not sure how this could be tested though. Just making sure that this one helper function does it right doesn't say much.

[ReimarBauer]

I think this different, when we haven't a test we need to communicate that often, while a test says "no, that is wrong".

Yes, what I am saying is just that we should make a conscious decision on what we test, and why, because overdoing it on the details is a net-negative. If we want to declare that all timestamps must be encoded with isoformat, then that is fine to add as a test; it then becomes part of the expected interface instead of remaining an implementation detail.

My stab at test_get_message_dict is meant to say that this test encodes that we must have a helper function called get_message_dict and that it must return a dictionary with a message_type key, while the value of that key must be MessageType.TEXT for the specific message we gave to it. That is hardly a useful tests, and it would just be a hindrance when refactoring that part of the code. Instead, the important thing to test is that a message sent from one client to another arrives unaltered, and if it uses this helper function on its way then we get coverage for it for free.

In principle it does pinning the definition of the get_message_dict, not detailed enough to use it for a TDD approach. Also not the origin version. When the lib would have been defined by TDD we likly would have there also very different quality of these tests now. That is maybe also an interesting aspect for a talk about the tests life cycle of a project by TDD. How it looks like for a young project and envolves on a mature project.
But back to this, when this would have been the TDD test for that function, we would maybe like it that we have it.
I don't know right now whether I want to reject tests that are now subsequently created in this form for existing code.

What we currently mostly do is to write tests afterwards for bugfixes or refactoring, for better understanding what it is doing and how and to get a coverage so it is harder for us to break something without realizing it. And important we continously improve.

[matrss]

Seems OK to me. I think this ad-hoc Message-to-dict mapper should eventually be replaced by a real serialization/deserialization module that can be properly tested with e.g. property-based testing.