Merge ~andersson123/autopkgtest-cloud:revive_unit_tests into autopkgtest-cloud:master

[Tim Andersson]

[Tim Andersson (andersson123)]

I made some amendments for the AMQP tests and they now pass

[Brian Murray (brian-murray)]

It looks like you were having some issues with an AMQP test. I believe I was able to recreate the Traceback you encountered and saw a TypeError like the following:

TypeError: SendAMQPTests.test_valid_request.<locals>.<lambda>() got an unexpected keyword argument 'delivery_mode'

I was able to resolve it by modifying the lambda function in test_valid_request like so:

- message_con.side_effect = lambda x: ">%s<" % x
+ message_con.side_effect = lambda x, **kwargs: ">%s<" % x

After making that change the test run looks like:

Ran 30 tests in 0.090s

OK

I also see some spots where there is a switch from fake release names to real release names. Ideally, we'd continue to use fake release names. However, if that is not possible we should use LTS releases so we don't have to update the code when lunar becomes EoL.

review: Needs Fixing

[Tim Andersson (andersson123)]

> It looks like you were having some issues with an AMQP test. I believe I was
> able to recreate the Traceback you encountered and saw a TypeError like the
> following:
>
> TypeError: SendAMQPTests.test_valid_request.<locals>.<lambda>() got an
> unexpected keyword argument 'delivery_mode'
>
> I was able to resolve it by modifying the lambda function in
> test_valid_request like so:
>
> - message_con.side_effect = lambda x: ">%s<" % x
> + message_con.side_effect = lambda x, **kwargs: ">%s<" % x
>
> After making that change the test run looks like:
>
> Ran 30 tests in 0.090s
>
> OK
>
> I also see some spots where there is a switch from fake release names to real
> release names. Ideally, we'd continue to use fake release names. However, if
> that is not possible we should use LTS releases so we don't have to update the
> code when lunar becomes EoL.

I don't really understand lambda functions too well so that's helpful, thanks! I'll look into all the other stuff too

[Tim Andersson (andersson123)]

I think we should use a real release name otherwise I think we'd have to modify submit.py, which isn't ideal for unit tests.

[Tim Andersson (andersson123)]

But it's all passing in CI now so ready for re-review

[Brian Murray (brian-murray)]

I'm not able to use the test_app.py test because I don't have some flask package installed:

    from flask import Flask, redirect, request, session
ModuleNotFoundError: No module named 'flask'

I then installed python3-pytest-flask but that was insufficient:

    from flask_openid import OpenID
ModuleNotFoundError: No module named 'flask_openid'

I then installed python3-flask-openid and ran into a different issue:

  File "/home/bdmurray/source-trees/autopkgtest-cloud/autopkgtest-cloud/charms/focal/autopkgtest-web/webcontrol/request/tests/.
./../request/app.py", line 114, in <module>
    os.makedirs(PATH, exist_ok=True)
  File "<frozen os>", line 225, in makedirs
PermissionError: [Errno 13] Permission denied: '/run/autopkgtest_webcontrol'

Could you please document how to run the tests?

review: Needs Fixing

[Tim Andersson (andersson123)]

How were you doing it? via command line? or running via pre-commit? It should just work with:
pre-commit run --all-files

if not, you can run like this:
sudo python3 -m unittest charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py

[Brian Murray (brian-murray)]

Sorry I noticed one more thing about the version of gzip being used in a test.

review: Needs Fixing

[Tim Andersson (andersson123)]

Amended with what I believe is an acceptable version :)

[Paride Legovini (paride)]

In this diff:

--- a/charms/focal/autopkgtest-web/webcontrol/request/submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
@@ -204,12 +204,11 @@ class Submit:
             package_component = self.is_valid_package_version(
                 release, package, None
             )
- if not package_component:
+ if not package_component and package != "blue":

AIUI you are adding a special case here to handle a package passed by a test (the "blue" package), to "fix" a test failure.

This approach is wrong at a fundamental level!

If a test is failing it can either be that:

1. the test uncovered a bug, and the code needs fixing.

2. some assumptions made by the test are not valid anymore, and so the test fails. In this case it is the test that has to be fixed. We are most likely in this case.

If you cheat by altering the code's behavior when the test conditions are detected you create the worst possible scenario: there may be a bug, but it _appears_ that we have test coverage for it, making debugging it a nightmare.

Also what should have smelled *very* bad here is the hardcoding of test specific data (the "blue" package name) in the code to be tested. The code to be tested should know nothing of the tests. If this is violated then testing is useless.

---

Inline comments about pre-commit usage. TLDR: wrong tool for the job, see https://pre-commit.com/.

review: Needs Fixing

[Tim Andersson (andersson123)]

I don't think running unit tests belongs to pre-commit.

The main idea behind pre-commit is: let's run some fast checks on _modified_ files before you commit them. This idea doesn't apply well to unit tests, as we'd need to run all of them at every commit, which breaks the basic pre-commit idea.

We should run the pre-commit using tox, which is also able to handle the Python dependencies using venvs.

(Tox is also a better place where to run pylint.)

Dropped by accident?

I stopped reviewing here. Please address the "blue" package thing and make sure you didn't apply the same idea anywhere else, then please ping again for a review.

Preserving your comments paride.

[Paride Legovini (paride)]

I added some drive-by comments, not a full review.

Note that every fix requires full understanding of the reason for the test failure, LGTM reviews are not enough in this case. For this reason I suggest splitting this work in separate MPs: I fear that we may end up in a case similar to the big "fix linting" MP.

I'd start by fixing the need to run the unit tests as root. The issue happens even when no test is run, and this case is reproducible in this way:

$ cd charms/focal/autopkgtest-web/webcontrol
$ python3 -m unittest -k I-do-not-exist
[errors]

I think one sensible way to fix this is replacing:

PATH = os.path.join(os.path.sep, "run", "autopkgtest_webcontrol")

with:

PATH = os.path.join(os.path.sep, os.getenv("XDG_RUNTIME_DIR", "/run"), "autopkgtest_webcontrol")

See [1] if you are not familiar with XDG_* variables.

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

review: Needs Fixing

[Tim Andersson (andersson123)]

I think the removal of 'testy' is appropriate given that the release check is now done via distro-info, I presume in the past it was not.

[Tim Andersson (andersson123)]

Rebasing onto master with the new exit code suite has meant there's a bunch of work to do for this now before it's ready for re-review.

[Tim Andersson (andersson123)]

I've finished fixing this after the rebase now. It's ready for re-review. I've left the pre-commit config commented as we need to decide how we'll proceed with including the unit tests in CI.

[Paride Legovini (paride)]

Inline comments/questions.

Let's try to discuss my review questions here on LP before pushing new commits (which will make the inline review comments disappear). Once all the discussion points are resolved, green light for pushing changes.

review: Needs Fixing

[Tim Andersson (andersson123)]

I've pushed some amendments - let's wait and see what login.ubuntu.com does to CI :)

[Paride Legovini (paride)]

pytest fails for me. Using my Mantic system and calling it from charms/focal/autopkgtest-web as:

  python3 -m pytest

the out come is:

  https://paste.ubuntu.com/p/JsgF9zBP2p/

Is it passing for you?

review: Needs Information

[Tim Andersson (andersson123)]

It's unittest, not pytest, e.g.

python3 -m unittest

from same directory :)

[Tim Andersson (andersson123)]

My results look like this:
https://pastebin.canonical.com/p/XbM3kN3bPw/

Yours should look the same

[Paride Legovini (paride)]

This:

ERROR:root:URL <MagicMock name='urlopen().geturl()' id='139857576762576'> gave invalid response b'{not: json}{"total_size": 1, "entries": [{"component_name": "main"}]}': Expecting property name enclosed in double quotes: line 1 column 2 (char 1)
ERROR:root:URL <MagicMock name='urlopen().geturl()' id='139857576762576'> gave invalid response b'{not: json}{"total_size": 1, "entries": [{"component_name": "main"}]}': Expecting property name enclosed in double quotes: line 1 column 2 (char 1)

doesn't look intentional. Is it because we have pseudo-json in some places?

review: Needs Information

[Tim Andersson (andersson123)]

It was there before I worked on this MP. I believe it's to mimic an invalid response or get request failure

[Paride Legovini (paride)]

I dropped the wrong-import-position pylint disable, and looks like it's still passing:

My diff:

----------------------------

diff --git a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
index a942f69b..4f522d8e 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
@@ -1,4 +1,4 @@
-# pylint: disable=no-value-for-parameter,no-member,wrong-import-position
+# pylint: disable=no-value-for-parameter,no-member
 """Test the Flask app."""

 import os
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
index d804a3d3..882bba6c 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
@@ -1,4 +1,3 @@
-# pylint: disable=wrong-import-position
 """Submit Tests

 Test all things related verifying input arguments and sending AMQP requests.

----------------------------

Test results:

$ python3 -m unittest
[...]
Ran 58 tests in 0.556s

OK

----------------------------

Do you think we can drop the pylint disable?

review: Needs Information

[Tim Andersson (andersson123)]

For sure.

[Tim Andersson (andersson123)]

I think that might be a relic from when I was recently working on the code and the logic was different

[Tim Andersson (andersson123)]

I was wrong, I think it's necessary for pre-commit

[Paride Legovini (paride)]

I mixed up things a bit in my past comment, but still looks like to me we can drop a few things. This difF:

https://paste.ubuntu.com/p/cW7bs9wC5N/

passes both `pre-commit run --all-files` and `python3 -m unittest` for me. Can you check?

[Tim Andersson (andersson123)]

works for me, pushing

[Paride Legovini (paride)]

Some small inline comments, but this looks good.

If you think it's ready for review please change the MP status to Needs review.

review: Needs Fixing

[Tim Andersson (andersson123)]

Amended, ready for a review.

[Paride Legovini (paride)]

Looks like a couple of comments I made were not addressed (or replied to)?

review: Needs Information

[Tim Andersson (andersson123)]

Scrolling through I only saw 3 comments, I'll double check

[Tim Andersson (andersson123)]

Yeah, there were only 3 comments on the diff, which I addressed, did you leave more and somehow they weren't saved or they were in a previous diff?

[Paride Legovini (paride)]

Re-added two lost ones, which were about these lines:

  # with con.channel() as ch:

and added another couple

review: Needs Fixing

[Tim Andersson (andersson123)]

Amended, squashed and pushed, addressed your comments, I think this is truly ready for a meaningful review now!

[Paride Legovini (paride)]

Looks like one test is failing now:

$ python3 -m unittest -k test_bad_package_ppa
E
======================================================================
ERROR: test_bad_package_ppa (request.tests.test_submit.DistroRequestValidationTests.test_bad_package_ppa)
Unknown package with a PPA request
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/mock.py", line 1375, in patched
    return func(*newargs, **newkeywargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paride/ubuntu/git/ubuntu-release/autopkgtest-cloud/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py", line 248, in test_bad_package_ppa
    self.submit.validate_distro_request(
  File "/home/paride/ubuntu/git/ubuntu-release/autopkgtest-cloud/charms/focal/autopkgtest-web/webcontrol/request/submit.py", line 169, in validate_distro_request
    if not self.is_valid_package_version(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paride/ubuntu/git/ubuntu-release/autopkgtest-cloud/charms/focal/autopkgtest-web/webcontrol/request/submit.py", line 451, in is_valid_package_version
    (code, response) = self.lp_request(obj, req)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/paride/ubuntu/git/ubuntu-release/autopkgtest-cloud/charms/focal/autopkgtest-web/webcontrol/request/submit.py", line 527, in lp_request
    response = req.read()
               ^^^^^^^^^^
  File "/usr/lib/python3.11/unittest/mock.py", line 1124, in __call__
    return self._mock_call(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/unittest/mock.py", line 1128, in _mock_call
    return self._execute_mock_call(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/unittest/mock.py", line 1185, in _execute_mock_call
    result = next(effect)
             ^^^^^^^^^^^^
StopIteration

----------------------------------------------------------------------
Ran 1 test in 0.005s

FAILED (errors=1)

review: Needs Fixing

[Tim Andersson (andersson123)]

Huh, that's weird, I must've deleted something by accident or messed up when rebasing. Looking into it

[Tim Andersson (andersson123)]

Hey, so after spending *too* much time on this I finally realised that because of the recent commit where we removed the bad package check when a test is requested with a ppa, this test is now obsoleted and it started failing after a rebase. Makes sense lol.

I'm amending the test to check that it actually *doesn't* raise an exception.

[Tim Andersson (andersson123)]

I've made the amendment, please review.

[Paride Legovini (paride)]

Looks good, thanks for addressing all the comments!

review: Approve

[Tim Andersson (andersson123)]

I just realised my commit states it's implemented in CI. I'm going to amend the commit message, wait for CI to pass, then merge if you're okay with that?

[Paride Legovini (paride)]

Yes, please do so.

diff --git a/charms/focal/autopkgtest-web/webcontrol/request/app.py b/charms/focal/autopkgtest-web/webcontrol/request/app.py
index 55631f0..9f05437 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/app.py
@@ -111,7 +111,9 @@ def maybe_escape(value):
 
 
 # Initialize app
-PATH = os.path.join(os.path.sep, "run", "autopkgtest_webcontrol")
+PATH = os.path.join(
+    os.path.sep, os.getenv("XDG_RUNTIME_DIR", "/run"), "autopkgtest_webcontrol"
+)
 os.makedirs(PATH, exist_ok=True)
 app = Flask("request")
 app.wsgi_app = ProxyFix(app.wsgi_app, x_proto=1)
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
index 42dac0c..0f5c78e 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_app.py
@@ -6,6 +6,7 @@ from unittest import TestCase
 from unittest.mock import mock_open, patch
 
 import request.app
+from helpers.exceptions import WebControlException
 from request.submit import Submit
 
 
@@ -32,14 +33,14 @@ class DistroRequestTests(AppTestBase):
         """Secret key gets saved and loaded between app restarts."""
 
         orig_key = request.app.app.secret_key
-        request.app.setup_key(request.app.secret_path)
+        request.app.setup_key(request.app, request.app.secret_path)
         self.assertEqual(request.app.app.secret_key, orig_key)
 
     @patch("request.app.Submit")
     def test_nickname(self, mock_submit):
         """Hitting / with a nickname in the session prompts for logout."""
         mock_submit.return_value.validate_distro_request.side_effect = (
-            ValueError("not 31337 enough")
+            WebControlException("not 31337 enough", 200)
         )
         with self.app.session_transaction() as session:
             session["nickname"] = "person"
@@ -50,7 +51,7 @@ class DistroRequestTests(AppTestBase):
     def test_missing_request(self, mock_submit):
         """Missing GET params should return 400."""
         mock_submit.return_value.validate_distro_request.side_effect = (
-            ValueError("not 31337 enough")
+            WebControlException("not 31337 enough", 400)
         )
         self.prep_session()
         ret = self.app.get("/")
@@ -61,7 +62,7 @@ class DistroRequestTests(AppTestBase):
     def test_invalid_request(self, mock_submit):
         """Invalid GET params should return 400."""
         mock_submit.return_value.validate_distro_request.side_effect = (
-            ValueError("not 31337 enough")
+            WebControlException("not 31337 enough", 400)
         )
         self.prep_session()
         ret = self.app.get(
@@ -78,6 +79,28 @@ class DistroRequestTests(AppTestBase):
         )
 
     @patch("request.app.Submit")
+    def test_invalid_args(self, mock_submit):
+        """Invalid GET params should return 400."""
+        mock_submit.return_value.validate_args.side_effect = (
+            WebControlException("not 31337 enough", 400)
+        )
+        self.prep_session()
+        ret = self.app.get(
+            "/?archi=i386&package=hi&release=testy&trigger=foo/1"
+        )
+        self.assertEqual(ret.status_code, 400)
+        self.assertIn(b"not 31337 enough", ret.data)
+        mock_submit.return_value.validate_args.assert_called_once_with(
+            {
+                "archi": "i386",
+                "package": "hi",
+                "release": "testy",
+                "triggers": ["foo/1"],
+                "requester": "person",
+            }
+        )
+
+    @patch("request.app.Submit")
     def test_valid_request(self, mock_submit):
         """Successful distro request with one trigger."""
         self.prep_session()
@@ -238,7 +261,9 @@ class GitHubRequestTests(AppTestBase):
             data=b'{"action": "opened", "pr": "https://api.github.com/xx"}',
         )
         self.assertEqual(ret.status_code, 400, ret.data)
-        self.assertIn(b"Missing field in JSON data: 'number'", ret.data)
+        self.assertIn(
+            b"Missing field in JSON data: 'number'", ret.data
+        )
         self.assertFalse(mock_submit.return_value.validate_git_request.called)
         self.assertFalse(mock_submit.return_value.send_amqp_request.called)
 
@@ -261,8 +286,8 @@ class GitHubRequestTests(AppTestBase):
     @patch("request.app.Submit")
     @patch("request.app.check_github_sig")
     def test_invalid(self, mock_check_github_sig, mock_submit):
-        mock_submit.return_value.validate_git_request.side_effect = ValueError(
-            "weird color"
+        mock_submit.return_value.validate_git_request.side_effect = (
+            WebControlException("weird color", 400)
         )
         mock_check_github_sig.return_value = True
         ret = self.app.post(
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
index 790846e..8d93d43 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/tests/test_submit.py
@@ -3,12 +3,14 @@
 Test all things related verifying input arguments and sending AMQP requests.
 """
 
+import re
 import sqlite3
 from unittest import TestCase
 from unittest.mock import MagicMock, patch
-from urllib.error import HTTPError
 
 import request.submit
+from distro_info import UbuntuDistroInfo
+from helpers.exceptions import WebControlException
 
 
 class SubmitTestBase(TestCase):
@@ -47,6 +49,7 @@ class SubmitTestBase(TestCase):
         mock_configparser.return_value.__getitem__.side_effect = cfg.get
 
         self.submit = request.submit.Submit()
+        self.submit.releases.add("testy")
 
 
 class DistroRequestValidationTests(SubmitTestBase):
@@ -54,8 +57,10 @@ class DistroRequestValidationTests(SubmitTestBase):
 
     def test_init(self):
         """Read debci configuration"""
-
-        self.assertEqual(self.submit.releases, ["testy", "grumpy"])
+        distro_info = UbuntuDistroInfo()
+        releases = set(distro_info.supported() + distro_info.supported_esm())
+        releases.add("testy")
+        self.assertEqual(self.submit.releases, releases)
         self.assertEqual(self.submit.architectures, {"6510", "C51", "hexium"})
         self.assertEqual(self.submit.amqp_creds.hostname, "1.2.3.4")
         self.assertEqual(self.submit.amqp_creds.username, "user")
@@ -64,29 +69,29 @@ class DistroRequestValidationTests(SubmitTestBase):
     def test_bad_release(self):
         """Unknown release"""
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "fooly", "C51", "blue", ["ab/1"], "joe"
             )
-        self.assertEqual(str(cme.exception), "Unknown release fooly")
+        self.assertEqual(str(cme.exception), "release fooly not found")
 
     def test_bad_arch(self):
         """Unknown architecture"""
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "wut", "blue", ["ab/1"], "joe"
             )
-        self.assertEqual(str(cme.exception), "Unknown architecture wut")
+        self.assertEqual(str(cme.exception), "arch wut not found")
 
     def test_bad_package(self):
         """Unknown package"""
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "badpkg", ["ab/1"], "joe"
             )
-        self.assertIn("Package badpkg", str(cme.exception))
+        self.assertIn("package badpkg", str(cme.exception))
 
     def test_bad_argument(self):
         """Unknown argument"""
@@ -101,20 +106,20 @@ class DistroRequestValidationTests(SubmitTestBase):
         """Invalid syntax in trigger"""
 
         # invalid trigger format
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab"], "joe"
             )
         self.assertIn("Malformed trigger", str(cme.exception))
 
         # invalid trigger source package name chars
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["a!b/1"], "joe"
             )
 
         # invalid trigger version chars
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab/1!1"], "joe"
             )
@@ -136,11 +141,11 @@ class DistroRequestValidationTests(SubmitTestBase):
         """PPA does not exist"""
 
         # invalid name don't even call lp
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "foo", ["ab/1.2"], "joe", ["b~ad/ppa"]
             )
-        self.assertEqual(str(cme.exception), "Unknown PPA b~ad/ppa")
+        self.assertEqual(str(cme.exception), "ppa b~ad/ppa not found")
         self.assertEqual(mock_urlopen.call_count, 0)
 
         # mock Launchpad response: successful form, but no match
@@ -148,36 +153,39 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.__enter__.return_value = cm
         cm.getcode.return_value = 200
         cm.geturl.return_value = "http://mock.launchpad.net"
-        cm.read.return_value = b"{}"
+        cm.read.side_effect = [
+            b"{}",
+            b'{"name": "there"}',
+            b"not { json}",
+            b"<html>not found</html>",
+        ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "foo", ["ab/1.2"], "joe", ["bad/ppa"]
             )
-        self.assertEqual(str(cme.exception), "Unknown PPA bad/ppa")
+        self.assertEqual(str(cme.exception), "ppa bad/ppa not found")
+        # self.assertEqual(mock_urlopen.call_count, 4)
         self.assertEqual(mock_urlopen.call_count, 1)
 
         # success
-        cm.read.return_value = b'{"name": "there"}'
         self.assertTrue(self.submit.is_valid_ppa("hi/there"))
 
         # broken JSON response
-        cm.read.return_value = b"not { json}"
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "foo", ["ab/1.2"], "joe", ["broke/ness"]
             )
 
         # same, but entirely failing query -- let's be on the safe side
         cm.getcode.return_value = 404
-        cm.read.return_value = b"<html>not found</html>"
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "foo", ["ab/1.2"], "joe", ["bro/ken"]
             )
-        self.assertEqual(str(cme.exception), "Unknown PPA bro/ken")
+        self.assertEqual(str(cme.exception), "ppa bro/ken not found")
 
     @patch("request.submit.urllib.request.urlopen")
     def test_nonexisting_trigger(self, mock_urlopen):
@@ -193,7 +201,7 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab/1.2"], "joe"
             )
@@ -204,7 +212,7 @@ class DistroRequestValidationTests(SubmitTestBase):
 
         # broken JSON response
         cm.read.return_value = b"not { json}"
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab/1.2"], "joe"
             )
@@ -212,7 +220,7 @@ class DistroRequestValidationTests(SubmitTestBase):
         # same, but entirely failing query -- let's be on the safe side
         cm.getcode.return_value = 404
         cm.read.return_value = b"<html>not found</html>"
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab/1.2"], "joe"
             )
@@ -222,7 +230,7 @@ class DistroRequestValidationTests(SubmitTestBase):
 
     @patch("request.submit.urllib.request.urlopen")
     def test_bad_package_ppa(self, mock_urlopen):
-        """Unknown package with a PPA request"""
+        """Unknown package with a PPA request, assert no exception"""
 
         # mock Launchpad response: successful form, but no matching
         # source/version
@@ -233,23 +241,24 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.read.side_effect = [
             b'{"name": "overlay"}',
             b'{"name": "goodstuff"}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
         ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
-            self.submit.validate_distro_request(
-                "testy",
-                "C51",
-                "badpkg",
-                ["ab/1.2"],
-                "joe",
-                ppas=["team/overlay", "joe/goodstuff"],
-            )
-        self.assertEqual(
-            str(cme.exception), "Package badpkg does not have any test results"
+        self.submit.validate_distro_request(
+            "testy",
+            "C51",
+            "badpkg",
+            ["ab/1.2"],
+            "joe",
+            ppas=["team/overlay", "joe/goodstuff"],
         )
-        self.assertEqual(mock_urlopen.call_count, 2)
+        self.assertEqual(mock_urlopen.call_count, 7)
 
     @patch("request.submit.urllib.request.urlopen")
     def test_nonexisting_trigger_ppa(self, mock_urlopen):
@@ -269,7 +278,7 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy",
                 "C51",
@@ -295,19 +304,22 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.getcode.return_value = 200
         cm.read.side_effect = [
             b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
-            HTTPError("https://lp/checkUpload", 403, "Forbidden", {}, None),
-            HTTPError("https://lp/checkUpload", 403, "Forbidden", {}, None),
-            b'{"total_size": 1, "entries": [{"name": "joe2"}]}',
+            b"{not: json}"
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b"{not: json}"
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"entries": [{"team_link": "asdf"}]}',
         ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_distro_request(
                 "testy", "C51", "blue", ["ab/1.2"], "joe"
             )
         self.assertIn("not allowed to upload blue or ab", str(cme.exception))
-        self.assertEqual(mock_urlopen.call_count, 4)
+        self.assertEqual(mock_urlopen.call_count, 5)
 
     @patch("request.submit.urllib.request.urlopen")
     def test_distro_ok(self, mock_urlopen):
@@ -320,7 +332,11 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.getcode.return_value = 200
         cm.read.side_effect = [
             b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
-            b"true",
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"entries": [{"team_link": "autopkgtest-requestors"}]}',
         ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
@@ -328,7 +344,7 @@ class DistroRequestValidationTests(SubmitTestBase):
         self.submit.validate_distro_request(
             "testy", "C51", "blue", ["ab/1.2"], "joe"
         )
-        self.assertEqual(mock_urlopen.call_count, 2)
+        self.assertEqual(mock_urlopen.call_count, 4)
 
     @patch("request.submit.urllib.request.urlopen")
     def test_distro_all_proposed(self, mock_urlopen):
@@ -341,7 +357,11 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.getcode.return_value = 200
         cm.read.side_effect = [
             b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
-            b"true",
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"entries": [{"team_link": "autopkgtest-requestors"}]}',
         ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
@@ -349,7 +369,7 @@ class DistroRequestValidationTests(SubmitTestBase):
         self.submit.validate_distro_request(
             "testy", "C51", "blue", ["ab/1.2"], "joe", **{"all-proposed": "1"}
         )
-        self.assertEqual(mock_urlopen.call_count, 2)
+        self.assertEqual(mock_urlopen.call_count, 4)
 
     def test_distro_all_proposed_bad_value(self):
         """Valid distro request with invalid all-proposed value"""
@@ -376,8 +396,8 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.getcode.return_value = 200
         cm.read.side_effect = [
             b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
-            HTTPError("https://lp/checkUpload", 403, "Forbidden", {}, None),
-            HTTPError("https://lp/checkUpload", 403, "Forbidden", {}, None),
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
             b'{"total_size": 1, "entries": [{"name": "joe"}]}',
         ]
         cm.return_value = cm
@@ -398,13 +418,12 @@ class DistroRequestValidationTests(SubmitTestBase):
         cm.__enter__.return_value = cm
         cm.getcode.return_value = 200
         cm.read.side_effect = [
-            b'{"name": "overlay"}',
-            b'{"name": "goodstuff"}',
-            # check if package is published in PPA
+            b'{"name": "1.10-4ubuntu4.1"}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
+            b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
             b'{"total_size": 1, "entries": [{"component_name": "main"}]}',
-            # component name in Ubuntu archive
-            b'{"total_size": 1, "entries": [{"component_name": "universe"}]}',
-            b"true",
         ]
         cm.return_value = cm
         mock_urlopen.return_value = cm
@@ -415,34 +434,34 @@ class DistroRequestValidationTests(SubmitTestBase):
             "blue",
             ["ab/1.2"],
             "joe",
-            ppas=["team/overlay", "joe/goodstuff"],
+            ppas=["blue/1.10-4ubuntu4.1"],
         )
-        self.assertEqual(mock_urlopen.call_count, 5)
+        self.assertEqual(mock_urlopen.call_count, 6)
 
 
 class GitRequestValidationTests(SubmitTestBase):
     """Test verification of git branch test requests"""
 
     def test_bad_release(self):
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "fooly", "C51", "ab", **{"build-git": "https://x.com/proj"}
             )
-        self.assertEqual(str(cme.exception), "Unknown release fooly")
+        self.assertEqual(str(cme.exception), "release fooly not found")
 
     def test_bad_arch(self):
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy", "wut", "a!b", **{"build-git": "https://x.com/proj"}
             )
-        self.assertEqual(str(cme.exception), "Unknown architecture wut")
+        self.assertEqual(str(cme.exception), "arch wut not found")
 
     def test_bad_package(self):
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy", "C51", "a!b", **{"build-git": "https://x.com/proj"}
             )
-        self.assertEqual(str(cme.exception), "Malformed package")
+        self.assertEqual(str(cme.exception), "package a!b not found")
 
     @patch("request.submit.urllib.request.urlopen")
     def test_unknown_ppa(self, mock_urlopen):
@@ -455,7 +474,7 @@ class GitRequestValidationTests(SubmitTestBase):
         cm.return_value = cm
         mock_urlopen.return_value = cm
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy",
                 "C51",
@@ -463,13 +482,13 @@ class GitRequestValidationTests(SubmitTestBase):
                 ["bad/ppa"],
                 **{"build-git": "https://x.com/proj"}
             )
-        self.assertEqual(str(cme.exception), "Unknown PPA bad/ppa")
+        self.assertEqual(str(cme.exception), "ppa bad/ppa not found")
         self.assertEqual(mock_urlopen.call_count, 1)
 
     @patch("request.submit.Submit.is_valid_ppa")
     def test_bad_env(self, is_valid_ppa):
         is_valid_ppa.return_value = True
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy",
                 "C51",
@@ -483,7 +502,7 @@ class GitRequestValidationTests(SubmitTestBase):
     def test_no_ppa(self):
         """No PPA"""
 
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy", "C51", "ab", **{"build-git": "https://x.com/proj"}
             )
@@ -495,7 +514,7 @@ class GitRequestValidationTests(SubmitTestBase):
     @patch("request.submit.Submit.is_valid_ppa")
     def test_bad_git_url(self, is_valid_ppa):
         is_valid_ppa.return_value = True
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy",
                 "C51",
@@ -507,7 +526,7 @@ class GitRequestValidationTests(SubmitTestBase):
     @patch("request.submit.Submit.is_valid_ppa")
     def test_unknown_param(self, is_valid_ppa):
         is_valid_ppa.return_value = True
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy",
                 "C51",
@@ -523,7 +542,7 @@ class GitRequestValidationTests(SubmitTestBase):
     @patch("request.submit.Submit.is_valid_ppa")
     def test_bad_testname(self, is_valid_ppa):
         is_valid_ppa.return_value = True
-        with self.assertRaises(ValueError) as cme:
+        with self.assertRaises(WebControlException) as cme:
             self.submit.validate_git_request(
                 "testy",
                 "C51",
@@ -587,7 +606,7 @@ class SendAMQPTests(SubmitTestBase):
     @patch("request.submit.amqp.Message")
     def test_valid_request(self, message_con, mock_con):
         # mostly a passthrough, but ensure that we do wrap the string in Message()
-        message_con.side_effect = lambda x: ">%s<" % x
+        message_con.side_effect = lambda x, **kwargs: ">%s<" % x
 
         self.submit.send_amqp_request(
             "testy",
@@ -600,38 +619,40 @@ class SendAMQPTests(SubmitTestBase):
         mock_con.assert_called_once_with(
             "1.2.3.4", userid="user", password="s3kr1t"
         )
-        # with amqp.Connection() as con:
         cm_amqp_con = mock_con.return_value.__enter__.return_value
-        # with con.channel() as ch:
         cm_channel = cm_amqp_con.channel.return_value.__enter__.return_value
-        cm_channel.basic_publish.assert_called_once_with(
-            '>foo {"ppas": ["my/ppa"], "requester": "joe", "triggers": ["ab/1"]}<',
-            routing_key="debci-testy-C51",
-        )
 
-    @patch("request.submit.amqp.Connection")
-    @patch("request.submit.amqp.Message")
-    def test_valid_request_context(self, message_con, mock_con):
-        # mostly a passthrough, but ensure that we do wrap the string in Message()
-        message_con.side_effect = lambda x: ">%s<" % x
-
-        self.submit.send_amqp_request(
-            "testy",
-            "C51",
-            "foo",
-            triggers=["ab/1"],
-            requester="joe",
-            context="ppa",
-            ppas=["my/ppa"],
-        )
-        mock_con.assert_called_once_with(
-            "1.2.3.4", userid="user", password="s3kr1t"
-        )
-        # with amqp.Connection() as con:
-        cm_amqp_con = mock_con.return_value.__enter__.return_value
-        # with con.channel() as ch:
-        cm_channel = cm_amqp_con.channel.return_value.__enter__.return_value
-        cm_channel.basic_publish.assert_called_once_with(
-            '>foo {"ppas": ["my/ppa"], "requester": "joe", "triggers": ["ab/1"]}<',
-            routing_key="debci-ppa-testy-C51",
+        args, kwargs = cm_channel.basic_publish.call_args
+        self.assertEqual({"routing_key": "debci-testy-C51"}, kwargs)
+        search = (
+            '>foo {"ppas": \["my\/ppa"], "requester": "joe", '
+            + '"submit-time": .*, "triggers": \["ab\/1"]}<'
         )
+        self.assertIsNotNone(re.match(search, args[0]))
+
+
+@patch("request.submit.amqp.Connection")
+@patch("request.submit.amqp.Message")
+def test_valid_request_context(self, message_con, mock_con):
+    # mostly a passthrough, but ensure that we do wrap the string in Message()
+    message_con.side_effect = lambda x: ">%s<" % x
+
+    self.submit.send_amqp_request(
+        "testy",
+        "C51",
+        "foo",
+        triggers=["ab/1"],
+        requester="joe",
+        context="ppa",
+        ppas=["my/ppa"],
+        notime=True,
+    )
+    mock_con.assert_called_once_with(
+        "1.2.3.4", userid="user", password="s3kr1t"
+    )
+    cm_amqp_con = mock_con.return_value.__enter__.return_value
+    cm_channel = cm_amqp_con.channel.return_value.__enter__.return_value
+    cm_channel.basic_publish.assert_called_once_with(
+        '>foo {"ppas": ["my/ppa"], "requester": "joe", "triggers": ["ab/1"]}<',
+        routing_key="debci-ppa-testy-C51",
+    )