1294234 - Run legacy tests with run-task

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Gregory Szorc [:gps]]

As part of running WPT from source checkout (bug 1286900), we'll need to port the TC tasks to use `run-task` so it can do the source checkout.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1294234 - Add test-ubuntu*.sh scripts to Docker image;

Before, we `curl`d these files at task run time. Why, I'm not sure.
Probably the easiest way to do things before we could add files outside
the directory of the Dockerfile to the image build context. Since that
is no longer a restriction, we just include the file as part of the
built image.

This does mean we rebuild the Docker image if the scripts change. I'm
fine with that.

It's worth noting that both test.sh files look very similar. I
anticipate consolidation in the future...

Review commit: https://reviewboard.mozilla.org/r/70778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70778/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1294234 - Assign command at end of function;

Previously, the code performed a "hidden" assignment to
worker['command'] when creating the "command" variable. This
tripped me up when reading the code because relying on variable
references isn't exactly obvious.

We now defer the assignment to worker['command'] until the end,
making the code a little easier to understand.

Review commit: https://reviewboard.mozilla.org/r/70780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70780/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1294234 - Use run-task for test tasks;

Before, test.sh (duplicated between the desktop-test and
desktop1604-test images) was dropping permissions, creating a workspace,
and executing test-linux.sh. This is functionality now provided by
run-task.

So, convert the test tasks to use run-task.

This patch is a bit more complicated than it should be because we
have to pass the main task command as a single argument to run-task
instead of as explicit arguments. This isn't too difficult to fix:
we just shell quote the arguments instead of creating a list. But
it is a bit annoying.

Since the test.sh files are no longer used after this change, they have
been deleted. The desktop-test image no longer has any files in the
bin/ directory, so the Dockerfile entry to copy those files has been
removed.

Review commit: https://reviewboard.mozilla.org/r/70782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70782/

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8779871 [details]
Bug 1294234 - Assign command at end of function;

https://reviewboard.mozilla.org/r/70780/#review68326

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8779872 [details]
Bug 1294234 - Use run-task for test tasks;

https://reviewboard.mozilla.org/r/70782/#review68328

::: taskcluster/taskgraph/transforms/tests/make_task_description.py:201
(Diff revision 1)
> +    # assemble the bash script in a list that will be joined by spaces.
> +    # Task references are allowed.
> +    args = [
> +        'mkdir -p /home/worker/workspace &&',
> +        'cd /home/worker/workspace &&',
> +        '/home/worker/bin/test-linux.sh',

Why not make test-linux.sh create and cd into this directory?  I think that would save you having to use `bash -cx` in the command, which would save you the need to shell-quote everything.

::: taskcluster/taskgraph/transforms/tests/make_task_description.py:236
(Diff revision 1)
>  
> -    worker['command'] = command
> +    worker['command'] = [
> +        '/home/worker/bin/run-task',
> +        '--',
> +        'bash', '-cx',
> +        # Allow variable substitution.

`# allow task references`

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8779870 [details]
Bug 1294234 - Add test-ubuntu*.sh scripts to Docker image;

https://reviewboard.mozilla.org/r/70778/#review68330

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8779870 [details]
Bug 1294234 - Add test-ubuntu*.sh scripts to Docker image;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70778/diff/1-2/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8779871 [details]
Bug 1294234 - Assign command at end of function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70780/diff/1-2/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8779872 [details]
Bug 1294234 - Use run-task for test tasks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70782/diff/1-2/

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8779872 [details]
Bug 1294234 - Use run-task for test tasks;

https://reviewboard.mozilla.org/r/70782/#review68396

::: taskcluster/taskgraph/transforms/tests/make_task_description.py:198
(Diff revision 2)
>  
>      # assemble the command line
> +    command = [
> +        '/home/worker/bin/run-task',
> +        '--',
> +        '/home/worker/bin/test.sh',

so clean!

(assuming it works)

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8779872 [details]
Bug 1294234 - Use run-task for test tasks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70782/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/710d37b610a8
Add test-ubuntu*.sh scripts to Docker image; r=dustin
https://hg.mozilla.org/integration/autoland/rev/3cfc4041d48d
Assign command at end of function; r=dustin
https://hg.mozilla.org/integration/autoland/rev/d118ab526ad8
Use run-task for test tasks; r=dustin

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Various tests are failing like https://treeherder.mozilla.org/logviewer.html#?job_id=1792100&repo=autoland

All backed out in https://hg.mozilla.org/integration/autoland/rev/d8dc463ad2af

Comment 14

[Gregory Szorc [:gps]]

This needs the first commit from bug 1294109.

I'll reland this tomorrow once the trees are happier.

Comment 15

[Gregory Szorc [:gps]]

Attachment: Bug 1294234 - Use more byte literals;

Without this, we potentially mix unicode literals and str and have
implicit type coercion, leading to UnicodeDecodeError or some such.
Oh, the joys of Python.

Review commit: https://reviewboard.mozilla.org/r/71306/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71306/

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 8779870 [details]
Bug 1294234 - Add test-ubuntu*.sh scripts to Docker image;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70778/diff/2-3/

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8779871 [details]
Bug 1294234 - Assign command at end of function;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70780/diff/2-3/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8779872 [details]
Bug 1294234 - Use run-task for test tasks;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70782/diff/3-4/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a536501b52e2
Use more byte literals; r=ekyle
https://hg.mozilla.org/integration/autoland/rev/527ab73d66dc
Add test-ubuntu*.sh scripts to Docker image; r=dustin
https://hg.mozilla.org/integration/autoland/rev/c72df0f64a7f
Assign command at end of function; r=dustin
https://hg.mozilla.org/integration/autoland/rev/25bc6ca8f19f
Use run-task for test tasks; r=dustin

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b17f42ecbc3c
Backed out changeset 25bc6ca8f19f for breaking TC tests

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a536501b52e2
https://hg.mozilla.org/mozilla-central/rev/527ab73d66dc
https://hg.mozilla.org/mozilla-central/rev/c72df0f64a7f

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8780637 [details]
Bug 1294234 - Use more byte literals;

This was reviewed in another bug.