Skip to content

tests: Add assertion for model.server_url in test_init#1618

Open
pushkarscripts wants to merge 1 commit intozulip:mainfrom
pushkarscripts:tests-add-server-url-assertion
Open

tests: Add assertion for model.server_url in test_init#1618
pushkarscripts wants to merge 1 commit intozulip:mainfrom
pushkarscripts:tests-add-server-url-assertion

Conversation

@pushkarscripts
Copy link
Copy Markdown

@pushkarscripts pushkarscripts commented Feb 20, 2026

What does this PR do, and why?

Adds the missing assertion for model.server_url in test_init, resolving the existing FIXME comment.

Also extends coverage with a parametrized test covering different base_url scheme and netloc combinations to ensure correct normalization behavior.

External discussion & connections

  • Discussed in #zulip-terminal in topic PR: Add test for model.server_url in test_init
  • Fully fixes #
  • Partially fixes issue #
  • Builds upon previous unmerged work in PR #
  • Is a follow-up to work in PR #
  • Requires merge of PR #
  • Merge will enable work on #

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

@zulipbot zulipbot added the size: XS [Automatic label added by zulipbot] label Feb 20, 2026
@pushkarscripts pushkarscripts force-pushed the tests-add-server-url-assertion branch from 5314751 to b9b39bb Compare February 20, 2026 15:07
Comment thread tests/model/test_model.py
@zulipbot zulipbot added size: S [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels Feb 28, 2026
@pushkarscripts pushkarscripts force-pushed the tests-add-server-url-assertion branch from 4df61fc to afad74c Compare February 28, 2026 11:53
Copy link
Copy Markdown
Collaborator

@clado clado left a comment

Choose a reason for hiding this comment

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

lgtm! thanks for your work on improving it!! :)
maybe @neiljp you want to take a look, especially as the original writer of the fixme?

Comment thread tests/model/test_model.py
@clado
Copy link
Copy Markdown
Collaborator

clado commented Mar 2, 2026

wait, quick addendum (I am "commit undisciplined" lol) but it may be a good idea to turn your two commits into one commit, since they're pretty much the same idea? (And, since I'd argue that it's only the second commit that truly resolved the fixme that was removed in the first commit.)

The Zulip documentation discussing squashing commits in this manner, but perhaps not in enough detail to follow if you've never done an interactive rebase before... Totally unsure whether that describes you or not, but want to encourage you to ask for help if you need it! 🫡

@pushkarscripts
Copy link
Copy Markdown
Author

wait, quick addendum (I am "commit undisciplined" lol) but it may be a good idea to turn your two commits into one commit, since they're pretty much the same idea? (And, since I'd argue that it's only the second commit that truly resolved the fixme that was removed in the first commit.)

The Zulip documentation discussing squashing commits in this manner, but perhaps not in enough detail to follow if you've never done an interactive rebase before... Totally unsure whether that describes you or not, but want to encourage you to ask for help if you need it! 🫡

Thanks for the suggestion.
That makes sense since the second commit completes the fix.

I’ll squash them into a single commit 👍

@pushkarscripts pushkarscripts force-pushed the tests-add-server-url-assertion branch from afad74c to 7edcf74 Compare March 3, 2026 15:32
Add missing assertion for model.server_url in test_init,
resolving the existing FIXME. Also extend coverage with a
parametrized test covering multiple base_url scheme and
netloc combinations.
@pushkarscripts pushkarscripts force-pushed the tests-add-server-url-assertion branch from 7edcf74 to 6007acb Compare March 3, 2026 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S [Automatic label added by zulipbot]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants