The default bug view has changed. See this FAQ.

Convert some hooks tests to Mercurial testing format

RESOLVED FIXED

Status

Developer Services
Mercurial: hg.mozilla.org
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gps, Unassigned)

Tracking

Details

MozReview Requests

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

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Mercurial has a testing format that focuses on CLI interactions (http://mercurial.selenic.com/wiki/WritingTests). It makes user interaction tests clear and easy-to-understand. It also more accurately tests components because you are doing end-to-end tests, as opposed to unit tests. I'd like to convert the hooks tests to this format.

In this bug, I'll write some basic .t tests to get the ball rolling. I'm hoping others will pick off where I leave off until hghooks/runtests.py is eliminated.
(Reporter)

Comment 1

3 years ago
Created attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
Attachment #8473416 - Flags: review?(emorley)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
/r/108 - Bug 1054112 - Tests for push\_printurls hook

While I was here, I added tests for the push_printurls hook.
(Reporter)

Comment 3

3 years ago
Comment on attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
/r/108 - Bug 1054112 - Tests for push\_printurls hook
/r/109 - Bug 1054112 - Convert pushlog hook tests into .t test

Adding tests for pushlog hook.
Attachment #8473416 - Flags: review?(ted)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
/r/108 - Bug 1054112 - Tests for push\_printurls hook
/r/109 - Bug 1054112 - Convert pushlog hook tests into .t test
/r/111 - Bug 1054112 - Remove some CR from prevent\_uuid\_changes.py
/r/112 - Bug 1054112 - Port prevent-uuid-changes hook to .t test
/r/113 - Bug 1054112 - Remove CR from prevent\_webidl\_changes
/r/114 - Bug 1054112 - Port prevent-webidl hook tests to .t format

Ported UUID and WebIDL changes hooks to .t files
Attachment #8473416 - Flags: review?(ehsan)
(Reporter)

Comment 5

3 years ago
I could probably convert the rest. But I saw bugs fly by regarding changing the commit message hook and I don't want to introduce a code conflict.
(Reporter)

Comment 6

3 years ago
Comment on attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
/r/108 - Bug 1054112 - Tests for push\_printurls hook
/r/109 - Bug 1054112 - Convert pushlog hook tests into .t test
/r/111 - Bug 1054112 - Remove some CR from prevent\_uuid\_changes.py
/r/112 - Bug 1054112 - Port prevent-uuid-changes hook to .t test
/r/113 - Bug 1054112 - Remove CR from prevent\_webidl\_changes
/r/114 - Bug 1054112 - Port prevent-webidl hook tests to .t format

Fixed test breakage with Mercurial <2.7 due to reliance on strip extension, which was extracted from mq in 2.7.

Updated

3 years ago
Depends on: 1054300
I can't review these because of bug 1054300, but in the mean time, what is the .t format?  I'm not familiar with it.  And why has the number of tests run decreased?

<quote-from-commit-message>
runtests.py before and after output:

Ran 34 tests in 2.583s
Ran 26 tests in 1.747s
</quote-from-commit-message>
Flags: needinfo?(gps)
r=me
Also, how do I run tests after this?  Before I used:

cd hghooks
hg clone /path/to/mercurial mercurial
export PYTHONPATH=mercurial
./runtests.py

Would that still work?
(Reporter)

Comment 10

3 years ago
.t tests are described at http://mercurial.selenic.com/wiki/WritingTests

In summary:

Lines beginning with "  $ " are executed in a shell
Lines beginning with "  " are expected output from the command that just executed
Lines beginning with ">>> " are inline Python (you can also test their output)
You can add "(re)" or "(glob)" to the end of output lines to send them against regexp or a globber

As you can see from the ports, these .t tests are testing *more* because they are capturing the output from commands. If the user experience changes, the tests will catch that.

FWIW, we can also still run .py tests if we need to. But for anything related to Mercurial commands, I'd rather we do .t testing.

The commit messages are quoting the remaining, legacy tests in hghooks/runtests.py. I'd like to get them all converted to .t tests (or Mercurial-style .py tests) and eliminate runtests.py so we have a single "harness."
(Reporter)

Comment 11

3 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Also, how do I run tests after this?  Before I used:


Read https://hg.mozilla.org/hgcustom/version-control-tools/file/default/README.rst. tl;dr

  ./create-test-environment
  source venv/bin/activate
  ./run-mercurial-tests.py -j8

You can also run tests against *every* version of Mercurial from 2.5.4 through 3.1 by passing --all-versions to run-mercurial-tests.py. It's quite nice.

Oh, and another benefit of the .t tests and Mercurial's test harness is the environment is self-contained and controlled. runtests.py is pulling in my ~/.hgrc and leading to non-idempotent nor consistent behavior.
Flags: needinfo?(gps)
This change looks fine to me. I'm no hghooks peer or maintainer, but this is pretty straight-forward stuff.
Er, that last comment was from:

https://reviewboard-dev.allizom.org/r/104/
(Reporter)

Comment 14

3 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/342a1bddd7a9
https://hg.mozilla.org/hgcustom/version-control-tools/rev/09c744458c68
https://hg.mozilla.org/hgcustom/version-control-tools/rev/086c1ad25d16
(Reporter)

Comment 15

3 years ago
Rebased WebIDL on top of the first patch and pushed that so https://ci.mozilla.org/job/version-control-tools/ is a little happier.
r=me
Attachment #8473416 - Flags: review?(emorley) → review+
(Reporter)

Comment 17

3 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/42b4b8f97dfa
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7f4e9b9b2783
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e4c22c0819a2
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4fbca8878852
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6f1da8433d97
(Reporter)

Comment 18

3 years ago
Landed all the patches.

I'd appreciate if others could finish converting the remaining tests in runtests.py.

bkero: this is something you could do to learn a bit more about how to develop for Mercurial.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/hgcustom/version-control-tools/rev/086c1ad25d16 was not reviewed by me.  I note that the review request here is still pending, not sure if that means you want me to do a post-landing review, or what?  Please let me know what you need from me, and adjust the flags accordingly.  Thanks.
Status: RESOLVED → REOPENED
Flags: needinfo?(gps)
Resolution: FIXED → ---
(Reporter)

Updated

3 years ago
Blocks: 1054517
(Reporter)

Comment 20

3 years ago
What was comment 8 then? Is this rbbz not sending the appropriate context in the bugzilla comment? We should fix that...

ehsan: please do a post-landing review of the WebIDL conversion.
Flags: needinfo?(gps) → needinfo?(smacleod)
(In reply to Gregory Szorc [:gps] from comment #20)
> What was comment 8 then? Is this rbbz not sending the appropriate context in
> the bugzilla comment? We should fix that...
> 
> ehsan: please do a post-landing review of the WebIDL conversion.

Comment 8 is from https://reviewboard-dev.allizom.org/r/113/

I have previously filed Bug 1033489 to deal with adding more context (linking to the review / comment from the cross posted review) but it was deemed not a blocker for deployment. Should be pretty easy to fix though.
Flags: needinfo?(smacleod)
(Reporter)

Updated

3 years ago
Blocks: 1054668
Ship It!

Updated

3 years ago
Attachment #8473416 - Flags: review?(ehsan) → review+
(Reporter)

Updated

3 years ago
Attachment #8473416 - Flags: review?(ehsan)
Attachment #8473416 - Flags: review+
(Reporter)

Comment 23

3 years ago
Comment on attachment 8473416 [details]
Review for review ID: bz://1054112/gps

/r/104 - Bug 1054112 - Install hghooks package into testing environment
/r/105 - Bug 1054112 - Convert try-mandatory hook tests into .t tests
/r/108 - Bug 1054112 - Tests for push\_printurls hook
/r/109 - Bug 1054112 - Convert pushlog hook tests into .t test
/r/111 - Bug 1054112 - Remove some CR from prevent\_uuid\_changes.py
/r/112 - Bug 1054112 - Port prevent-uuid-changes hook to .t test
/r/113 - Bug 1054112 - Remove CR from prevent\_webidl\_changes
/r/114 - Bug 1054112 - Port prevent-webidl hook tests to .t format
(Reporter)

Comment 24

3 years ago
What the eff, ReviewBoard. I backspaced ted's email and it removed edmorley!
(Reporter)

Updated

3 years ago
Attachment #8473416 - Flags: review?(ted)
Attachment #8473416 - Flags: review?(ehsan)
(Reporter)

Comment 25

3 years ago
This landed and has Ehsan's review, so closing (again).

On the ReviewBoard front, I think we need to have explicit test coverage that the Bugzilla interactions are sane.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
I'm not really wild about the .t test format, but I'm also not actively doing hook development, so I'm not going to be stop energy. I do think that it sucks to have to learn yet another test harness format just to write tests...
(Reporter)

Comment 27

3 years ago
It's verbose, I concede that. But on the positive side, you actually test user interactions. I think that's a huge win. One only has to look at our lack of test coverage in mach land to understand the importance of these types of tests.

Also, you can still write .py unittest-based Mercurial tests. It's just the emphasis is on high-level .t tests. When we get around to e.g. writing a unified commit message parser, those will almost certainly be done as .py unit tests.
Honestly I am not a fan of this .t thing either.  It seems to be full of magic that I don't understand.  I would probably write future hook tests in python as I did before, I believe that the python test gives me everything that I care about (which is basically to validate my poor python coding skillz)
(Assignee)

Updated

3 years ago
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.