zulip-terminal
zulip-terminal copied to clipboard
zulip
Refactor Tests.
@sumanthvrao Some of this follows your ideas in #374, so it would be good to hear your thoughts on this.
For streams and users I have tried to trim to the data we are not using in tests. Do we really need all the data for testing? If we add a feature in the future that uses some data not available in the fixtures, then we can simply add that.
For changes within tests, I am not sure what would be the best way to show it as I have changed the stream/user id's in conftest. The id changes are required since in future if we can easily generate say 10 streams by increasing the range in the streams_fixtures or giving n as no. of streams to return.
Do you want me to squash the Add and Use fixtures?
Re 'all the data', I'm not suggesting detailed specific users like we had before - I like the general ones. However, my understanding is that the cross-realm bots are intrinsic to the server (typically, at least). Also, the subscriptions data seems incomplete compared to before - this was why I was trying to suggest having new fixtures with removal of old hard-coded data and use of the new fixtures in the same commit, to show what code is substituted.
If squashing those two commits looks reasonable, then I'd say give it a try (you can always keep a copy).
(The fixture naming is less important, but I thought it might be useful to see the context)
@amanagr To keep this moving and make it easier to manage I've pushed three commits to master now.
Is the avatar_url consistent? Is it now absent due to the client_gravatar setting?
I was going to merge the streams_fixture commit too, but it is causing some tests to fail for me, and the tests were failing generally too on this PR, so that may be related.
I've not looked at the time-based commit; in principle I like the idea, but it makes the tests less reproducible, so I would be happy using a 'seed' value instead, a little like we used to have?
Yeah, the avatar_url is not required since we will never use it, it's just dead data. I will try and fix the tests, strangely the tests passed last time I pushed changes and now without any changes made by me they have severely broken!
@amanagr Re the avatar_url, I was interested in whether this is data which we now don't receive, or which we just don't process. If the former, there is zero issue in just removing it; if the latter, I'm inclined to put a 'placeholder' in there, maybe even with a comment on each line as to why it's still there? In other cases we have kept lots of fields (eg for users or streams?) which we are not currently using. I'm not suggesting we'll use the avatar images in the foreseeable future - though we could! - but rather that while we've simplified/abstracted the fixtures received from the server, they should still map to the data we receive.
One possibility that I thought of while looking at this, would be a tool that sets up a 'fresh' dev install in a certain way, and then fetches candidate fixture elements that can be compared to what we have. That would allow tracking across API versions, extra fields, etc., and ensuring we don't lose any data that may (or may not) be there. That also sounds like the thoughts from before of having a 'template dev system' to get automated screenshots from, but let's not dive into that too fast yet :)
@amanagr I realized we hadn't merged these; I've pushed the avatar_url with an extra note in the summary, but could we skip what seems to be unnecessary mocking in the other commit?
Heads up @amanagr, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.
