1054112 - Convert some hooks tests to Mercurial testing format

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Gregory Szorc [:gps]]

Attachment: 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

Comment 2

[Gregory Szorc [:gps]]

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.

Comment 3

[Gregory Szorc [:gps]]

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.

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Gregory Szorc [:gps]]

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.

Comment 6

[Gregory Szorc [:gps]]

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.

Comment 7

[(no longer active)]

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>

Comment 8

[(no longer active)]

r=me

Comment 9

[(no longer active)]

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?

Comment 10

[Gregory Szorc [:gps]]

.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."

Comment 11

[Gregory Szorc [:gps]]

(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.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

This change looks fine to me. I'm no hghooks peer or maintainer, but this is pretty straight-forward stuff.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Er, that last comment was from:

https://reviewboard-dev.allizom.org/r/104/

Comment 14

[Gregory Szorc [:gps]]

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

Comment 15

[Gregory Szorc [:gps]]

Rebased WebIDL on top of the first patch and pushed that so https://ci.mozilla.org/job/version-control-tools/ is a little happier.

Comment 16

[Ed Morley [:emorley]]

r=me

Comment 17

[Gregory Szorc [:gps]]

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

Comment 18

[Gregory Szorc [:gps]]

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.

Comment 19

[(no longer active)]

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.

Comment 20

[Gregory Szorc [:gps]]

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 21

[Steven MacLeod [: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.

Comment 22

[(no longer active)]

Ship It!

Comment 23

[Gregory Szorc [:gps]]

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

Comment 24

[Gregory Szorc [:gps]]

What the eff, ReviewBoard. I backspaced ted's email and it removed edmorley!

Comment 25

[Gregory Szorc [:gps]]

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.

Comment 26

[(not currently active) Ted Mielczarek]

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...

Comment 27

[Gregory Szorc [:gps]]

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.

Comment 28

[(no longer active)]

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)