982852 - Make run_test() in xpcshell tests optional

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Gregory Szorc [:gps]]

xpcshell tests require the test file to define a run_test() function. If said function doesn't exist, an error is raised.

This requirement (likely) sprung up before add_test() and add_task() existed. Nowadays, most new xpcshell tests define their run_test() as:

function run_test() {
  run_next_test();
}

I'm tired of writing meaningless boilerplate code. We should allow run_test() to not exist if add_test() or add_task() have been called. This means a bare minimum xpcshell test file can consist of a single add_test() or add_task() call with no extra boilerplate.

Throwing this up in the mentored bug pool. This requires a working Firefox build and some (hopefully simple) modifications to testing/xpcshell/head.js and some basic tests in testing/xpcshell/selftest.py.

Needinfo :gps or :ted if you have any questions.

Comment 1

[Abhirav Kariya]

hi! I'm new here , Please assign this bug to me!

Comment 2

[Marco Bonardo [:mak]]

Looks like Gregory is mentoring this, likely he didn't notice the comment. Please use needinfo? flag (below the comment field) to get attention from the mentors.

Comment 3

[Gregory Szorc [:gps]]

Indeed, I missed it. Please use needinfo to get my attention.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

resetting the assigned to field as this has been idle for a few months.

Comment 5

[Krishnashish Gogoi [:doomkookie]]

Hello,

I'm looking to contribute towards my first bug in Mozilla. Can someone please assign this bug to me? I've looked over this a bit and feel that I can patch this up. I've tried out a few changes and some test cases in selftest.py and so far it seems to be working out fine.

Can someone please help me determine what should be passed as the string to do_test_pending() and do_test_finished() when run_test() is not defined? Right now it says "MAIN run_test", I believe it is logged out when a test is run. So, what should it be replaced with if run_test is not present? Or should it be left as it is (giving the impression that run_test() somehow is called)?

Comment 6

[Gregory Szorc [:gps]]

Our test harnesses are typically pretty sensitive to changes in output. It's probably best to make the output backwards compatible. But, the best test is whether it passes our automation. If automation is happy, I'm happy. We can find out if automation is happy by submitting your patch. Then, someone at Mozilla can send it to automation for you.

I'm on a soft holiday for the next few days. I may not respond to needinfo requests in a timely manner. Feel free to reach out to :ted.mielczarek or :chmanchester for questions until I get back.

Comment 7

[Krishnashish Gogoi [:doomkookie]]

Attachment: This patch checks if run_test is defined, if defined calls it, else calls run_next_test(). This makes run_test() optional in tests.

First patch for this bug. Changes head.js so that run_test() becomes optional for tests from now. I had a hard time getting my head around mercurial, so if the patch is in a wrong format, please inform me. The next 3 patches provide the test cases for this.

Comment 8

[Krishnashish Gogoi [:doomkookie]]

Attachment: Basic test cases added. Runs without run_test defined.

Comment 9

[Krishnashish Gogoi [:doomkookie]]

Attachment: More test cases. These check that tests fail as expected.

Comment 10

[Krishnashish Gogoi [:doomkookie]]

Attachment: Last test case. Checks that multiple add_task tests work without run_test.

Comment 11

[Krishnashish Gogoi [:doomkookie]]

Not sure if I should have broken up the patches. I'm not used to mercurial so I just ran 'hg export -r "outgoing()" -o "%n_%m.patch"'. If this is wrong, please let me know. I didn't read that mozreview required commit access, so I spent a bit of time there. If more test cases are required, I should be able to add them if these aren't wrong.

I have no idea on how to get this to automation, so I'll need help there as well, just some pointing in the general direction. :3

Thank you.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8588019 [details] [diff] [review]
This patch checks if run_test is defined, if defined calls it, else calls run_next_test(). This makes run_test() optional in tests.

Review of attachment 8588019 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8588022 [details] [diff] [review]
Basic test cases added. Runs without run_test defined.

Review of attachment 8588022 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/selftest.py
@@ +970,5 @@
> +
> +    def testNoRunTestEmptyTest(self):
> +        """
> +        Check that the test passes on an empty file that contains neither
> +        run_test() nor add_test(), add_task().

I'm not thrilled with this behavior. But I think changing the behavior of the test harness to emit error when no test functions are defined falls under the realm of a different bug. Until then, garbage in -> garbage out applies and this behavior is tolerable. (It even looks like existing behavior you just happened to stumble upon by writing thorough tests!)

Comment 14

[Gregory Szorc [:gps]]

This series looks great!

I'll push these changes to our test automation before landing. (The patches look fine but I've learned to never land changes to test harnesses without testing things first - weird things tend to happen.)

You shouldn't have to do anything unless automation says something is wrong with the patches.

Comment 15

[Gregory Szorc [:gps]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6617259056d

Comment 16

[Gregory Szorc [:gps]]

The test run looks happy. So I'm going to land this shortly.

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/4f35292a39b4
https://hg.mozilla.org/integration/fx-team/rev/bacbce5ae187
https://hg.mozilla.org/integration/fx-team/rev/561aca98f306
https://hg.mozilla.org/integration/fx-team/rev/621b3a17b9f7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f35292a39b4
https://hg.mozilla.org/mozilla-central/rev/bacbce5ae187
https://hg.mozilla.org/mozilla-central/rev/561aca98f306
https://hg.mozilla.org/mozilla-central/rev/621b3a17b9f7

Comment 19

[Gregory Szorc [:gps]]

These patches are now integrated into the mainline of Firefox. There's nothing left to do on this bug.

Thanks again for the excellent patches!

Comment 20

[Krishnashish Gogoi [:doomkookie]]

Awesome! I didn't know before that automation referred to the Try server.

Thanks for the review and all the help :D This has been a great experience for me.