Add test coverage for all URL endpoints #1441

Closed
showell opened this Issue Jul 28, 2016 · 8 comments

Projects

None yet

2 participants

@showell
Contributor
showell commented Jul 28, 2016

Once #1440 hits master (or even before then, if you want to work off of that branch), you'll be able to produce a report of URL endpoints that aren't covered by tools/test-backend.

We should do one of the following with each endpoint:

  • If we still support it, ideally we should add a test for it, and then follow up with the traditional code coverage reports to make sure we thoroughly cover the associated view (and ideally all its callees).
  • If we still support it, but there's some logistical reason we can't test it, we should flag it so that it only shows up as a warning in the report. (The report isn't very polished yet, so this would possibly require building better reporting on top of #1440.)
  • If we truly don't support the endpoint anymore, including all of our possible external API users, we should remove it from urls.py, or at least further deprecate it in some way.
@showell
Contributor
showell commented Jul 28, 2016 edited

The report will look something like this when you run ./tools/test-backend:

Searching for unused patterns...
listing them...
  ^accounts/login/google/$
  ^accounts/login/google/done/$
  ^accounts/login/jwt/$
  ^accounts/login/local/$
  ^accounts/login/sso/$
  ^accounts/password/done/$
  ^accounts/password/reset/$
  ^accounts/password/reset/(?P<uidb64>[0-9A-Za-z]+)/(?P<token>.+)/$
  ^accounts/password/reset/done/$
  ^accounts/send_confirm/(?P<email>[\S]+)?
  ^accounts/webathena_kerberos_login/
  ^api/$
  ^api/endpoints/$
  ^api/v1/external/bitbucket$
  ^api/v1/external/desk$
  ^apps/$
  ^desktop_home/$
  ^en/
  ^features/$
  ^hello/$
  ^integrations/$
  ^invite/$
  ^json/fetch_raw_message$
  ^json/get_events$
  ^json/language_setting$
  ^json/messages_in_narrow$
  ^json/report_error$
  ^json/report_narrow_time$
  ^json/report_send_time$
  ^json/report_unnarrow_time$
  ^json/tutorial_send_message$
  ^login/$
  ^new-user/$
  ^notify_tornado$
  ^register/$
  ^register/(?P<domain>\S+)/$
  ^robots\.txt$
  ^static/(?P<path>.*)$
@showell
Contributor
showell commented Jul 28, 2016

Since some of these endpoints are going to take significant work to either get them covered or research whether we can safely eliminate them, there's a good interim task here that's a bit easier.

We should have a follow up commit to #1440 that enforces that there are no regressions on which endpoints are covered. In other words, make it so that ./tools/test-backend does coverage checks by default, and if any endpoints other than the known endpoints above do not have coverage, the test suite should fail, and Travis should also fail.

@timabbott It may make sense to break this out into a separate issue, but I'm adding it here for context.

@timabbott
Member

Maybe it's worth updating this list now that we've added coverage for a bunch of these? It may be that most of what's left is the logged-out URLs, which would be really fast....

@timabbott
Member
timabbott commented Oct 16, 2016 edited

OK I just checked this and indeed we're now down to a very small number of untested URLs:

$ cat var/untested_url_report.txt
untested urls
  ^about/$
  ^accounts/login/jwt/$
  ^accounts/login/local/$
  ^accounts/login/sso/$
  ^accounts/password/done/$
  ^accounts/password/reset/done/$
  ^accounts/send_confirm/(?P<email>[\S]+)?
  ^desktop_home/$
  ^en/
  ^json/get_events$
  ^notify_tornado$
  ^register/(?P<domain>\S+)/$
  api/v1/external/deskdotcom
@timabbott
Member

To follow up on these items:

  • I opened an issue for the deskdotcom integration. (#2031)
  • I just added a test for about/.
  • I just added a test for /en/.
  • I just added a test for /desktop_home.
  • I don't understand why our existing password reset test (PasswordResetTest) isn't providing coverage for ^accounts/password/reset/done/$; and its friend need investigation. Same with the /accounts/send_confirm one; I'm pretty sure we go through that flow in a test...
  • /json/get_events I think we should be able to replace with the similar REST API endpoint and eliminate the old endpoint.
  • We should open issues for adding tests for the SSO, JWT, and local authentication login methods.
  • We should open an issue for adding a test for the /register/<domain> feature (or eliminate it).
  • notify_tornado is interesting because it's only use if we're using Tornado in a separate process, which we don't in the backend test suite. It does get tested properly by the Casper tests, but it probably makes sense to add some sort of test for it in the backend suite anyway.

So here's the current status

  ^accounts/login/jwt/$
  ^accounts/login/local/$
  ^accounts/login/sso/$
  ^accounts/password/done/$
  ^accounts/password/reset/done/$
  ^accounts/send_confirm/(?P<email>[\S]+)?
  ^json/get_events$
  ^notify_tornado$
  ^register/(?P<domain>\S+)/$
  api/v1/external/deskdotcom [has an open issue]
@timabbott
Member

Opened the auth issue as #2042

@timabbott
Member

I resolved /json/get_events just now, and #2108 resolves the auth stuff, which leaves us with just:

  api/v1/external/deskdotcom [has an open issue being worked on]
  ^accounts/password/done/$
  ^accounts/password/reset/done/$
  ^accounts/send_confirm/(?P<email>[\S]+)?
  ^notify_tornado$
  ^register/(?P<domain>\S+)/$

notify_tornado actually should have a test added, but the /accounts stuff is I think an issue with the tracking; @showell when you get a chance do you want a take a crack at fixing the tracking issue there?

I think that /register/domain endpoint is a feature that we probably want to just remove now that we have the subdomains feature, which is a better version of it.

@showell
Contributor
showell commented Oct 28, 2016

@timabbott On the accounts stuff, I think we decided that tracking redirects can be kind of difficult because that happens pretty deep in Django testing. So I think the best option here is to directly test those endpoints with negative tests that verify the redirected-to endpoints don't have loopholes. I can start in on this early next week.

@timabbott timabbott added this to the Zulip roadmap milestone Nov 18, 2016
@timabbott timabbott added a commit that closed this issue Nov 19, 2016
@showell @timabbott showell + timabbott tests: Enforce 100% URL coverage.
We now instrument URL coverage whenever you run the back end tests,
and if you run the full suite and fail to test all endpoints, we
exit with a non-zero exit code and report failures to you.

If you are running just a subset of the test suite, you'll still
be able to see var/url_coverage.txt, which has some useful info.

With some tweaks to the output from tabbott.

Fixes #1441.
5f5e6b6
@timabbott timabbott closed this in 5f5e6b6 Nov 19, 2016
@brockwhittaker brockwhittaker added a commit to brockwhittaker/zulip that referenced this issue Nov 30, 2016
@showell @brockwhittaker showell + brockwhittaker tests: Enforce 100% URL coverage.
We now instrument URL coverage whenever you run the back end tests,
and if you run the full suite and fail to test all endpoints, we
exit with a non-zero exit code and report failures to you.

If you are running just a subset of the test suite, you'll still
be able to see var/url_coverage.txt, which has some useful info.

With some tweaks to the output from tabbott.

Fixes #1441.
9b2a2d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment