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

Split test_ui_tools to multiple smaller test files. #422

Closed
wants to merge 4 commits into from

Conversation

sumanthvrao
Copy link
Member

The reason for this change is to

  • make it wasy to find position to add new tests (Especially for new contributors)
  • To make the test structure similar to code structure

@zulipbot zulipbot added the size: XL [Automatic label added by zulipbot] label Jul 8, 2019
@sumanthvrao
Copy link
Member Author

@amanagr @neiljp A word of caution that if we decide to go ahead with this approach, this would cause conflicts with most of the PRs we have on going.

@neiljp
Copy link
Collaborator

neiljp commented Jul 8, 2019

@sumanthvrao Zulip server has used a tool to work with that issue, which I think is TinglingGit, though I've not used it and it may be overkill here. We should possibly just try and review/merge some more first, though that tool may indicate which would be good to reduce conflicts on merging this.

@sumanthvrao
Copy link
Member Author

Great! I'm looking into it.

@sumanthvrao
Copy link
Member Author

Here's an output of running the tool for tests/ui/test_ui_tools.py:

tests/ui/test_ui_tools.py
[PR#387(+7-7)](https://github.com/zulip/zulip-terminal/pull/387) 
[PR#400(+4-46)](https://github.com/zulip/zulip-terminal/pull/400)
[PR#278(+46-0)](https://github.com/zulip/zulip-terminal/pull/278)
[PR#419(+0-5)](https://github.com/zulip/zulip-terminal/pull/419)
[PR#315(+145-1)](https://github.com/zulip/zulip-terminal/pull/315)
[PR#335(+6-2)](https://github.com/zulip/zulip-terminal/pull/335)
[PR#354(+129-2)](https://github.com/zulip/zulip-terminal/pull/354)
[PR#358(+36-1)](https://github.com/zulip/zulip-terminal/pull/358)
[PR#367(+34-0)](https://github.com/zulip/zulip-terminal/pull/367)
[PR#382(+211-5)](https://github.com/zulip/zulip-terminal/pull/382)

Around 10 open PRs have changes involving some parts of test_ui_tools.py

@neiljp
Copy link
Collaborator

neiljp commented Jul 28, 2019

@sumanthvrao How are the conflicts looking like now?

@sumanthvrao
Copy link
Member Author

@neiljp I believe they are still the same amount of conflicts which would probably appear if we got merged.

sumanthvrao added a commit to sumanthvrao/zulip-terminal that referenced this pull request Mar 20, 2020
* This commit introduces the change, while the actual linking has
been done in a follow up commit.
* Tests for views have been moved to a separate folder, along the
lines of zulip#422.
* The update_user_list function has been marked x-fail for now.
@zulipbot
Copy link
Member

Heads up @sumanthvrao, 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.

Base automatically changed from master to main January 30, 2021 20:30
@neiljp
Copy link
Collaborator

neiljp commented May 30, 2024

This was a useful investigation 👍

However, this has been open a long time, and we made this transition initially starting some tests in separate files, and moving them across in fa2fbed for buttons, and d5a22ba for messages (into a distinct test_messages.py).

It would likely still be useful to make changes such as:

  • move view element tests (eg. StreamsView, MessagesView) from ui/test_ui_tools into ui_tools/test_views (matching existing code positions)
  • split popup code out from ui_tools/views into ui_tools/popups (matching existing test split)

I've filed the latter suggestions as #1508 so this isn't lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests has conflicts size: XL [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants