Status

Conduit
Transplant
RESOLVED FIXED
3 years ago
24 days ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

3 years ago
Most of the Autoland tests were written prior to having reviewboard and mercurial in docker containers. We're now in a position to be able to write much better integration tests and we should do so before we start landing revisions to an inbound tree.
(Assignee)

Updated

3 years ago
Depends on: 1175146
(Assignee)

Updated

3 years ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8630450 [details]
MozReview Request: testing: link autoland container to hgrb (bug 1174752) r?gps

testing: link autoland container to hgrb (bug 1174752) r?gps
Attachment #8630450 - Flags: review?(gps)
(Assignee)

Comment 2

3 years ago
Created attachment 8630451 [details]
MozReview Request: testing: create autoland ldap user for testing (bug 1174752) r?gps

testing: create autoland ldap user for testing (bug 1174752) r?gps
Attachment #8630451 - Flags: review?(gps)
(Assignee)

Comment 3

3 years ago
Created attachment 8630452 [details]
MozReview Request: testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

Autoland services jobs asynchronously so the status commands are more useful if
we poll until the jobs either succeed or fail.
Attachment #8630452 - Flags: review?(mcote)
(Assignee)

Comment 4

3 years ago
Created attachment 8630453 [details]
MozReview Request: testing: add autoland startup test (bug 1174752) r?mcote

testing: add autoland startup test (bug 1174752) r?mcote
Attachment #8630453 - Flags: review?(mcote)
(Assignee)

Comment 5

3 years ago
Created attachment 8630454 [details]
MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r=mdoglio

autoland: provide better default for missing repo path (bug 1174752) r?mdoglio
Attachment #8630454 - Flags: review?(mdoglio)
(Assignee)

Comment 6

3 years ago
Created attachment 8630455 [details]
MozReview Request: testing: clone created repos in autoland container (bug 1174752) r?gps

testing: clone created repos in autoland container (bug 1174752) r?gps
Attachment #8630455 - Flags: review?(gps)
(Assignee)

Comment 7

3 years ago
Created attachment 8630456 [details]
MozReview Request: testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

Update the autoland tests to make use of the repository cloned from hgrb rather
than the "fake" repository that was used previously.
Attachment #8630456 - Flags: review?(mdoglio)
Attachment #8630454 - Flags: review?(mdoglio) → review+
Comment on attachment 8630454 [details]
MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r=mdoglio

https://reviewboard.mozilla.org/r/12731/#review11373

::: autoland/autoland/transplant.py:16
(Diff revision 1)
> +                                   os.path.join(os.path.sep + 'repos', tree))

I find os.path.join('', 'repos', tree) a bit more readable. Feel free to drop this comment if you don't agree.
Comment on attachment 8630456 [details]
MozReview Request: testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

https://reviewboard.mozilla.org/r/12735/#review11375

::: autoland/tests/test-post-autoland-job.t:20
(Diff revision 1)
> +  pushing to ssh://172.17.42.1:$HGPORT6/test-repo

Where does this ip come from?

::: autoland/tests/test-post-autoland-job.t:84
(Diff revision 1)
> -Get pullrequest job status
> +Posting a pullrequest job without a bugid should automatically file a bug for the user
>  
> -  $ ottoland pullrequest-job-status $AUTOLAND_URL 2
> -  (200, u'{\n  "bugid": 1, \n  "destination": "mozreview", \n  "error_msg": *, \n  "landed": *, \n  "pullrequest": 1, \n  "repo": "repo", \n  "result": *, \n  "user": "user"\n}') (glob)
> +  $ ottoland post-pullrequest-job $AUTOLAND_URL user repo 1 test-repo 1 cookie http://localhost:9898
> +  (200, u'{\n  "request_id": 4\n}')
> +  $ ottoland pullrequest-job-status $AUTOLAND_URL 4
> +  (200, u'{\n  "bugid": null, \n  "destination": "test-repo", \n  "error_msg": null, \n  "landed": null, \n  "pullrequest": 1, \n  "repo": "repo", \n  "result": "", \n  "user": "user"\n}')

Is there a way to verify that the bug was actually created?
Attachment #8630456 - Flags: review?(mdoglio)
(Assignee)

Comment 10

3 years ago
https://reviewboard.mozilla.org/r/12735/#review11375

> Is there a way to verify that the bug was actually created?

Right now this will always fail because I have a fake cookie. I'm planning to fix this when we have Bugzilla tokens. I'll add a TODO for now.
Comment on attachment 8630450 [details]
MozReview Request: testing: link autoland container to hgrb (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12723/#review11429

::: testing/vcttesting/docker.py:699
(Diff revision 1)
>              start_autoland = True

You'll want to add `start_hgrb = True` here.

::: testing/vcttesting/docker.py:846
(Diff revision 1)
> +                bind_path = os.path.abspath(os.path.dirname(self._state_path))

bind_path is unused, so you might as well kill it.
Attachment #8630450 - Flags: review?(gps) → review+

Updated

3 years ago
Attachment #8630451 - Flags: review?(gps) → review+
Comment on attachment 8630451 [details]
MozReview Request: testing: create autoland ldap user for testing (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12725/#review11435

::: testing/docker/builder-autoland/autoland_id_rsa:1
(Diff revision 1)
> +-----BEGIN RSA PRIVATE KEY-----

I'm not crazy about checking in pre-generated SSH keys. But since this is testing only, I don't think it matters too much. It certainly makes things simpler.

Updated

3 years ago
Attachment #8630455 - Flags: review?(gps)
Comment on attachment 8630455 [details]
MozReview Request: testing: clone created repos in autoland container (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12733/#review11437

::: testing/docker/builder-autoland/Dockerfile:20
(Diff revision 1)
> -  python2.7-dev libffi6 libffi-dev libpq-dev libldap2-dev libsasl2-dev \
> +  python2.7-dev mercurial libffi6 libffi-dev libpq-dev ca-certificates

You may find it easier to `pip install Mercurial`. That's actually preferred, as distros tend to not have the latest/greatest Mercurial version. And, you can explicitly control which version gets installed via pinning. It will require python-dev and possibly other support packages to compile, however.

::: testing/docker/builder-autoland/Dockerfile:32
(Diff revision 1)
> +ADD extra/vct/hgext/bootstrap.py ${VCT_HOME}/hgext/bootstrap.py
> +ADD extra/vct/hgext/reviewboard ${VCT_HOME}/hgext/reviewboard
> +ADD extra/vct/pylib/ ${VCT_HOME}/pylib/

This feels brittle. But it's a lesser evil than copying all of v-c-t over.

::: testing/docker/builder-autoland/Dockerfile:48
(Diff revision 1)
> -RUN mkdir /repos && hg init /repos/mozilla-central && hg init /repos/try
> +RUN cd /home/ubuntu && hg clone https://bitbucket.org/marmoute/mutable-history 

mutable-history is very aggressive about dropping compatibility with older versions. You'll almost certainly want to pin the Mercurial package version and the checked out mutable-history revision.
Comment on attachment 8630452 [details]
MozReview Request: testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

https://reviewboard.mozilla.org/r/12727/#review11525

::: testing/vcttesting/autoland_mach_commands.py:136
(Diff revision 1)
> +    def pullrequest_job_status(self, host, requestid, poll):
> +        url = host.rstrip('/') + '/pullrequest/mozreview/status/' + requestid
> +        if not poll:
> +            r = requests.get(url)
> +            print(r.status_code, r.text)
> +        else:
> +            import json
> +            import time
> +            attempts = 0
> +            while attempts < MAX_POLL_ATTEMPTS:
> +                attempts += 1
> +                r = requests.get(url)
> +                if r.status_code != 200 or json.loads(r.text)['landed'] is not None:
> -        print(r.status_code, r.text)
> +                    print(r.status_code, r.text)
> +                    break
> +                time.sleep(POLL_INTERVAL)
> +            else:
> +                print('timed out')

This code is duplicated exactly aside from the url. Screams to me that this should be put into its own function.
Attachment #8630452 - Flags: review?(mcote)
Comment on attachment 8630453 [details]
MozReview Request: testing: add autoland startup test (bug 1174752) r?mcote

https://reviewboard.mozilla.org/r/12729/#review11527

::: autoland/tests/test-post-autoland-job.t:8
(Diff revision 1)
> +  $ mozreview exec autoland tail -n 20 /home/ubuntu/autoland.log
> +   0:00.00 LOG: MainThread INFO starting autoland

Is there any reason you are tailing the last 20 lines but verifying there's only one?  Just to make sure no other messages are logged at startup?
Attachment #8630453 - Flags: review?(mcote)
(Assignee)

Comment 16

3 years ago
https://reviewboard.mozilla.org/r/12729/#review11527

> Is there any reason you are tailing the last 20 lines but verifying there's only one?  Just to make sure no other messages are logged at startup?

I'm just trying to provide enough context to look at a caught exception in the log without having to output the entire log file, which can grow big quickly if there's a fundamental problem with the code.
https://reviewboard.mozilla.org/r/12725/#review11539

::: hgext/reviewboard/tests/helpers.sh:69
(Diff revision 1)
> +  mozreview create-ldap-user autoland@example.com password 2000 'Otto Land' \
> +    --key-file $TESTDIR/testing/docker/builder-autoland/autoland_id_rsa \
> +    --scm-level 3 > /dev/null

the `mozreview` bind user is created as part of the container, so that the ldap container always mimics production in the accounts it has etc.

Should this be moved and created there as well? That way other tests which don't use helpers.sh have it.
(Assignee)

Comment 18

3 years ago
Comment on attachment 8630450 [details]
MozReview Request: testing: link autoland container to hgrb (bug 1174752) r?gps

testing: link autoland container to hgrb (bug 1174752) r?gps
Attachment #8630450 - Flags: review+ → review?(gps)
(Assignee)

Comment 19

3 years ago
Comment on attachment 8630451 [details]
MozReview Request: testing: create autoland ldap user for testing (bug 1174752) r?gps

testing: create autoland ldap user for testing (bug 1174752) r?gps
Attachment #8630451 - Flags: review+ → review?(gps)
(Assignee)

Comment 20

3 years ago
Comment on attachment 8630452 [details]
MozReview Request: testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

Autoland services jobs asynchronously so the status commands are more useful if
we poll until the jobs either succeed or fail.
Attachment #8630452 - Flags: review?(mcote)
(Assignee)

Updated

3 years ago
Attachment #8630453 - Flags: review?(mcote)
(Assignee)

Comment 21

3 years ago
Comment on attachment 8630453 [details]
MozReview Request: testing: add autoland startup test (bug 1174752) r?mcote

testing: add autoland startup test (bug 1174752) r?mcote
(Assignee)

Comment 22

3 years ago
Comment on attachment 8630454 [details]
MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r=mdoglio

autoland: provide better default for missing repo path (bug 1174752) r=mdoglio

This default helps with testing by allowing us to use repositories at paths
we do not know in advance.
Attachment #8630454 - Attachment description: MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r?mdoglio → MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r=mdoglio
Attachment #8630454 - Flags: review+ → review?(mdoglio)
(Assignee)

Updated

3 years ago
Attachment #8630455 - Flags: review?(gps)
(Assignee)

Comment 23

3 years ago
Comment on attachment 8630455 [details]
MozReview Request: testing: clone created repos in autoland container (bug 1174752) r?gps

testing: clone created repos in autoland container (bug 1174752) r?gps
(Assignee)

Comment 24

3 years ago
Comment on attachment 8630456 [details]
MozReview Request: testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

Update the autoland tests to make use of the repository cloned from hgrb rather
than the "fake" repository that was used previously.
Attachment #8630456 - Flags: review?(mdoglio)
https://reviewboard.mozilla.org/r/12725/#review11593

::: testing/docker/builder-ldap/mozilla.ldif:86
(Diff revision 2)
> +homeDirectory: /home/password
> +sn: Land
> +uid: password

the uid password thing confuses me? why isn't this something like `uid: autoland`?
Comment on attachment 8630450 [details]
MozReview Request: testing: link autoland container to hgrb (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12723/#review11595

Ship It!
Attachment #8630450 - Flags: review?(gps) → review+
(Assignee)

Comment 27

3 years ago
https://reviewboard.mozilla.org/r/12725/#review11593

> the uid password thing confuses me? why isn't this something like `uid: autoland`?

I used the slapcat utility to dump what was being generated by the create-ldap-user command I was using before. I didn't have a close look at the output until now.
Comment on attachment 8630451 [details]
MozReview Request: testing: create autoland ldap user for testing (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12725/#review11619

The "password" value for uid is the only thing that bothers me. Else, this is fine.
Attachment #8630451 - Flags: review?(gps) → review+
Comment on attachment 8630455 [details]
MozReview Request: testing: clone created repos in autoland container (bug 1174752) r?gps

https://reviewboard.mozilla.org/r/12733/#review11623

::: testing/docker/builder-autoland/Dockerfile:15
(Diff revision 2)
> +RUN apt-get -y install software-properties-common
> +RUN add-apt-repository ppa:mercurial-ppa/releases

I don't believe this isn't needed now that you pip install Mercurial.
Attachment #8630455 - Flags: review?(gps) → review+
Comment on attachment 8630452 [details]
MozReview Request: testing: add poll option to mach autoland status commands (bug 1174752) r?mcote

https://reviewboard.mozilla.org/r/12727/#review11659

Ship It!
Attachment #8630452 - Flags: review?(mcote) → review+

Updated

3 years ago
Attachment #8630453 - Flags: review?(mcote) → review+
Comment on attachment 8630453 [details]
MozReview Request: testing: add autoland startup test (bug 1174752) r?mcote

https://reviewboard.mozilla.org/r/12729/#review11661

Ship It!
Comment on attachment 8630454 [details]
MozReview Request: autoland: provide better default for missing repo path (bug 1174752) r=mdoglio

https://reviewboard.mozilla.org/r/12731/#review11699

Ship It!
Attachment #8630454 - Flags: review?(mdoglio) → review+
Comment on attachment 8630456 [details]
MozReview Request: testing: Update autoland tests to use clone of hgrb repositories (bug 1174752) r?mdoglio

https://reviewboard.mozilla.org/r/12735/#review11701

Ship It!
Attachment #8630456 - Flags: review?(mdoglio) → review+
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Product: Tree Management → MozReview

Updated

24 days ago
Product: MozReview → Conduit
You need to log in before you can comment on or make changes to this bug.