canonical-ubuntu-qa team mailing list archive

[Skia <mp+469607@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

First round of comments. Don't hesitate to ask for a pair-programming session to get those sorted, we might find some other improvements here and there along the way.

Diff comments:

> diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> index 3029f4f..b080e64 100755
> --- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> +++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
> @@ -756,6 +758,70 @@ def package_release_arch(package, release, arch, _=None):
>      )
>  
>  
> +@app.route("/run/<uuid>")
> +def get_by_uuid(uuid):
> +    results = []
> +    package = ""
> +    release = ""
> +    arch = ""
> +    cursor = db_con.cursor()
> +    cursor.row_factory = sqlite3.Row
> +    for row in cursor.execute(

Iterating here is semantically weird: the whole point of UUID is that they're unique, so the query result is actually binary: empty of not empty with a single element. I think something like `if query.empty(): raise NotFound else process the rest` would be good for this view.

> +        "SELECT run_id, version, triggers, duration, exitcode, requester, "
> +        "env, uuid, package, release, arch FROM result "
> +        "LEFT JOIN test ON result.test_id = test.id "
> +        "WHERE uuid = ?",
> +        (uuid,),
> +    ):
> +        requester = row["requester"] if row["requester"] else "-"
> +        code = human_exitcode(row["exitcode"])
> +        version = row["version"]
> +        triggers = row["triggers"]
> +        additional_params = row[
> +            "env"
> +        ]  # string of comma separated env variables e.g. all-proposed=1,test-name=mytest

I think the long line of comment is unnecessary, especially in this view that only does some display: it doesn't even care what's the information to be displayed.

> +        package = row["package"]
> +        release = row["release"]
> +        arch = row["arch"]
> +
> +        show_retry = code != "pass"

Maybe to be discussed with Brian, but always displaying the button would be an improvement to me: it allows getting a base URL that you sometimes don't want to run directly, but modify to your needs, and sometimes if all tests are pass, getting that URL can be difficult.

> +        url = os.path.join(
> +            swift_container_url % release,
> +            release,
> +            arch,
> +            srchash(package),
> +            package,
> +            row["run_id"],
> +        )
> +        all_proposed = (
> +            additional_params is not None
> +            and "all-proposed=1" in additional_params
> +        )
> +        results.append(
> +            (
> +                version,
> +                triggers,
> +                additional_params,
> +                human_date(row["run_id"]),
> +                human_sec(int(row["duration"])),
> +                requester,
> +                code,
> +                url,
> +                show_retry,
> +                all_proposed,
> +                row["uuid"],
> +            )
> +        )
> +    return render(
> +        "browse-test.html",

`browse-run.html` would be more consistent with the URL. But if the URL is meant to change, it will need to change too.

> +        package=package,
> +        release=release,
> +        arch=arch,
> +        test_results=results[0] if len(results) > 0 else None,
> +        title_suffix="- %s/%s/%s" % (package, release, arch),
> +    )
> +
> +
>  @app.route("/releases/<release>")
>  @app.route("/releases/<release>/<arch>")
>  def release(release, arch=None):
> diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-test.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-test.html
> new file mode 100644
> index 0000000..7bcd286
> --- /dev/null
> +++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-test.html
> @@ -0,0 +1,114 @@
> +{% extends "browse-layout.html" %}
> +{% import "macros.html" as macros %}
> +
> +{% block content %}
> +  <h2>
> +    <a href="{{base_url}}packages/{{package}}">{{package}}</a> <small>[{{release}}/{{arch}}]</small>
> +  </h2>
> +  <ul class="external-links links">
> +    <li>{{ macros.launchpad_link(package, release) }}</li>
> +    <li>{{ macros.excuses_link(package, release) }}</li>
> +  </ul>
> +
> +  {% if test_results %}

With the previous comment about the `if empty: raise NotFound`, this is not even needed.

> +    <table class="table">
> +      <tr>
> +        <th>
> +          <b>Version</b>

This is semantically wrong to bold all those titles like that: that's something to do from CSS, because you're already specifying that those are headers with the `<th>` (table header). Maybe adding a `test_result` (`test_run`?) class to the table for an easy select would help if you can't select it otherwise.

This comment is assuming basic knowledge of HTML/CSS, but if you don't understand what I mean, please come reach out and I'll explain further.

> +        </th>
> +
> +        <td>{{ test_results[0] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Triggers</b>
> +        </th>
> +
> +        <td>{{ test_results[1] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Env</b>
> +        </th>
> +
> +        <td>{{ test_results[2] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Date</b>
> +        </th>
> +
> +        <td>{{ test_results[3] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Duration</b>
> +        </th>
> +
> +        <td>{{ test_results[4] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Requester</b>
> +        </th>
> +
> +        <td>
> +
> +          {% if test_results[5] != "-" %}
> +            <a href="https://launchpad.net/~{{ test_results[5] }}">{{ test_results[5] }}</a>
> +          {% else %}
> +            {{ test_results[5] }}
> +          {% endif %}
> +
> +        </td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>Result</b>
> +        </th>
> +        <td class="nowrap {{ test_results[6] }}" title={{ test_results[6] }}>{{ test_results[6] }}

Missing double quote around `title` value

> +        </td>
> +      </tr>
> +      <tr>
> +
> +        <th>
> +          <b>UUID</b>
> +        </th>
> +
> +        <td>{{ test_results[10] }}</td>
> +      </tr>
> +      <tr>
> +
> +        <th></th>
> +
> +        <td>
> +
> +          {% if test_results[6] not in ["running", "queued"] %}
> +            <a href="{{ test_results[7] }}/log.gz">log</a> &emsp;
> +            <a href="{{ test_results[7] }}/artifacts.tar.gz">artifacts</a> &emsp;
> +
> +          {% endif %}
> +
> +          {% if test_results[8] %}

Not for this MP unless you want to, but we need to put that in a macro at some point!

> +            {% set ts = test_results[1].split()|map('urlencode')|join("&trigger=")|safe %}
> +            {% if test_results[9] %}
> +              <a href="{{ url_for("index_root") }}request.cgi?release={{ release }}&arch={{ arch }}&package={{ package|urlencode }}&trigger={{ ts }}&all-proposed=1">&#9851;</a>
> +            {% else %}
> +              <a href="{{ url_for("index_root") }}request.cgi?release={{ release }}&arch={{ arch }}&package={{ package|urlencode }}&trigger={{ ts }}">&#9851;</a>
> +            {% endif %}
> +          {% endif %}
> +
> +        </td>
> +      </tr>
> +    </table>
> +  {% else %}
> +    {{ macros.nonexistent_package() }}
> +  {% endif %}
> +
> +{% endblock content %}
> diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> index 0a585c5..70141d6 100644
> --- a/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> +++ b/charms/focal/autopkgtest-web/webcontrol/templates/macros.html
> @@ -119,7 +119,9 @@
>    </td>
>    <td class="nowrap {{ code }}" title={{ code }}>{{ code }}
>    </td>
> -  <td>{{uuid}}</td>
> +  <td>
> +    <a href="{{ url_for("index_root") }}run/{{uuid}}">{{uuid}}</a>

`url_for("get_by_uuid", uuid=uuid)` is the correct way to get the URL (even if the rest of the app doesn't necessarily do that :D)

> +  </td>
>    <td class="nowrap">
>      {% if code not in ["running", "queued"] %}
>        <a href="{{ url }}/log.gz">log</a> &emsp;


-- 
https://code.launchpad.net/~uralt/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/469607
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~uralt/autopkgtest-cloud:get-by-uuid into autopkgtest-cloud:master.