Skip to content

Add test-backend from Zulip server. #123

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add test-backend from Zulip server. #123

wants to merge 3 commits into from

Conversation

rht
Copy link
Contributor

@rht rht commented Sep 18, 2017

test pr works!

@rht rht force-pushed the test-api-2 branch 7 times, most recently from 3b8486b to 2b4a438 Compare September 19, 2017 04:53
@rht rht changed the title [wip!] Add test-backend from Zulip server. Add test-backend from Zulip server. Sep 19, 2017
@rht rht force-pushed the test-api-2 branch 4 times, most recently from d49ffbe to 4335b09 Compare September 19, 2017 05:09
@timabbott
Copy link
Member

@rht what's the thinking behind running the Zulip server's backend tests here?

@rht
Copy link
Contributor Author

rht commented Sep 20, 2017

This is to automate the integration test to check for PRs which require such, such as #18. The caveat is that it will slow down the aggregate CI test time. The other option is to add this commit only for the PRs where this test is needed, so the test is run once only.

The integration test will be required once again to test the stage 2 of Slack data import.

@timabbott
Copy link
Member

@derAnfaenger @eeshangarg what are your thoughts on this?

@timabbott
Copy link
Member

I think this might actually be a fine way to test for changes here that impact the integration with the backend...

@roberthoenig
Copy link
Collaborator

This is really cool, but it brings up the effective testing time from 1 minute to 11 minutes and goes over the magic five test jobs. Ultimately, I think we should merge this, but if it isn't urgent, maybe with a tweak similar to facebook/react#2000 ? Here, Travis checks the diff of the changes before running a suite - this would allow us to run the backend tests iff e.g. zulip/zulip is modified.

@rht
Copy link
Contributor Author

rht commented Oct 4, 2017

@derAnfaenger agreed on the test duration and the limitation of the parallel tests ! Should the test be triggered in main only when https://github.com/zulip/python-zulip-api/tree/master/zulip/zulip is patched?

I really need this to test the slack data import PR (this (#908) has been in the roadmap for ages who knows when).

@roberthoenig
Copy link
Collaborator

Testing when zulip/zulip is patched would be a god start, yeah. We could then later refine the exact directories which are affected. For slack, I guess you can just follow your own suggestion for now and add this commit to the PR for test purposes?

@roberthoenig
Copy link
Collaborator

@rht would it be sufficient in this case to just run tools/test-api?

@rht
Copy link
Contributor Author

rht commented Oct 5, 2017

Ahh, right, right, let me check how long the test takes with just tools/test-api.

@rht rht force-pushed the test-api-2 branch 6 times, most recently from 50bde01 to 54df05a Compare October 5, 2017 04:41
@rht rht force-pushed the test-api-2 branch 8 times, most recently from 7fbaa7a to 6e55581 Compare October 5, 2017 11:29
@roberthoenig
Copy link
Collaborator

@rht what's the state of this? We're possibly doing some iterations on the api code in the next days, so it'd be good to have the api tests set up by then. If you're working on other things right now, feel free to tell and I'd be happy to finish this :)

@rht
Copy link
Contributor Author

rht commented Oct 7, 2017

The only blocker is that I was testing between various Git clone ssh paths:

  1. sed -i "s|[email protected]|python-zulip-api.git@pull/$TRAVIS_PULL_REQUEST|" requirements/dev_lock.txt
  2. sed -i "s|zulip/[email protected]|$TRAVIS_PULL_REQUEST_SLUG.git@$TRAVIS_PULL_REQUEST_BRANCH|" requirements/dev_lock.txt
  3. sed -i "s|zulip/[email protected]|$TRAVIS_PULL_REQUEST_SLUG.git/$TRAVIS_PULL_REQUEST_BRANCH|" requirements/dev_lock.txt

@rht
Copy link
Contributor Author

rht commented Oct 7, 2017

Most likely wrong URL formatting. If you have any idea on what's going on, and unblock this, I'm totally up for this.

@rht
Copy link
Contributor Author

rht commented Feb 9, 2018

I will have to update this to use the circleci script.

@timabbott
Copy link
Member

We can also mitigate the performance impact here by having the tool only run the subset of backend tests that's related to the subsystem.

@zulipbot
Copy link
Member

zulipbot commented Sep 3, 2019

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

@timabbott
Copy link
Member

@PIG208 I think this would be a good effort to pick up. We probably won't end up reusing much of the exact code here, because we've moved to GitHub Actions since then, but I do think it'd be really nice if we can run the relevant part of the server's test suites against new python-zulip-api commits in CI. The main things I'm imagining are:

  • tools/test-api -- this is the most valuable, since it involves actually running the examples in our API documentation.
  • tools/test-backend x y z where we pass just the modules like BotStorage that integrate with the API project.

but once we have this setup, we can add other things as well.

kmill added a commit to kmill/python-zulip-api that referenced this pull request Jan 10, 2024
Also fixes a bug where the match variable `m` wasn't being reset in
the unlikely event that the message was completely empty.
Kha pushed a commit to Kha/python-zulip-api that referenced this pull request Jan 11, 2024
Also fixes a bug where the match variable `m` wasn't being reset in
the unlikely event that the message was completely empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants