Bug 63479 – webkitpy unit tests should have more descriptive names than just "Test"

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

webkitpy unit tests should have more descriptive names than just "Test"

[Adam Barth]

Created attachment 98780 [details]
Patch

[Dirk Pranke]

Comment on attachment 98780 [details]
Patch

The changes look fine, but aren't the names scoped to the module? I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done.

Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so.

[Adam Barth]

> The changes look fine, but aren't the names scoped to the module?

It depends on how they're imported.  As it happens, we import them with their fully qualified names, so there's no conflict.  However, if we imported them all into one flat namespace, we'd get in trouble.

> I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done.

Yeah, this mostly just code cleanliness patch.  "Test" is just a generic name.

> Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so.

My view is that this is mostly a matter of consistency.  Currently some of the unit tests files have this clause at the bottom and some don't, which makes it unpredictable whether running the file directly will do anything sensible.

Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests?  Currently, you need to pass the fully qualified class name, which is less than ideal.  Something like the gtest_filter is something we could emulate.

[Adam Barth]

Comment on attachment 98780 [details]
Patch

Clearing flags on attachment: 98780

Committed r89852: <http://trac.webkit.org/changeset/89852>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

(In reply to comment #3)
> > The changes look fine, but aren't the names scoped to the module?
> 
> It depends on how they're imported.  As it happens, we import them with their fully qualified names, so there's no conflict.  However, if we imported them all into one flat namespace, we'd get in trouble.
> 
> > I would think it would only be a problem if someone was importing "*" from multiple modules, which is not AFAIK normally done.
> 
> Yeah, this mostly just code cleanliness patch.  "Test" is just a generic name.
> 

Yup, I agree, which is why I approved it :).

> > Also, I notice that you are deleting the "if __name__ == '__main__' clauses ... as I've mentioned before, I use that functionality all the time and would prefer you not delete them unless you had a strong reason to do so.
> 
> My view is that this is mostly a matter of consistency.  Currently some of the unit tests files have this clause at the bottom and some don't, which makes it unpredictable whether running the file directly will do anything sensible.
>

I also agree with this, and have made it a practice in the past to add them where they were missing when I was otherwise changing a file. Of course, if others are practicing the opposite strategy, this can only end in unhappiness.

> Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests?  Currently, you need to pass the fully qualified class name, which is less than ideal.  Something like the gtest_filter is something we could emulate.

Possibly; it would certainly be better. In my personal work habits I usually have a shell open and in the directory containing the test files (or maybe one dir up), so "python foo_unittest.py" is easier than "test-webkitpy path-to-test-file" would be, usually (if test-webkitpy was smart enough to figure things out from the current working directory and a relative name, that would be fine). E.g. 'test-webkitpy foo' could look for foo_*test.py files in the current working dir.

Also running the tests directly integrates nicely with the python coverage package; it's been a while since I tried test-webkitpy to see if that also did, but I'd want to preserve that if possible.

[Adam Barth]

Another issue is that test_webkitpy sets up the environment for the tests.  For example, it configures a bunch of logging settings.  When you run the tests individually, you're running them in a different environment.

This all just seems like an elaborate workaround for the fact that the tests are slow.  If test-webkitpy ran in zero seconds, there would be no reason not to run the whole suite.

[Eric Seidel (no email)]

(In reply to comment #6)
> > Perhaps the best path forward is to improve test-webkitpy to make it easier to run individual tests?  Currently, you need to pass the fully qualified class name, which is less than ideal.  Something like the gtest_filter is something we could emulate.
> 
> Possibly; it would certainly be better. In my personal work habits I usually have a shell open and in the directory containing the test files (or maybe one dir up), so "python foo_unittest.py" is easier than "test-webkitpy path-to-test-file" would be, usually (if test-webkitpy was smart enough to figure things out from the current working directory and a relative name, that would be fine). E.g. 'test-webkitpy foo' could look for foo_*test.py files in the current working dir.
> 
> Also running the tests directly integrates nicely with the python coverage package; it's been a while since I tried test-webkitpy to see if that also did, but I'd want to preserve that if possible.

Interesting, I was not aware of the coverage usecase.

Clearly running a test directly with python test.py is differnet from running test-webkitpy.  Both in import constraints as well as logging (there are probably other ways too).  So I've been less excited about supporting that mode.

But perhaps there are arguments like the coverage argument which better support this individual mode.

I'd like to either:

- teach webkipy to run them all individually, like you're proposing doing, and auto-add some sort of main
- remove the ability to run them individually, and instead add all the individual-run features to test-webkitpy

Having two ways to run the tests seems only to end in sadness (like how it's possible to run them under multiple versions of python does today).

[Dirk Pranke]

(In reply to comment #7)
> Another issue is that test_webkitpy sets up the environment for the tests.  For example, it configures a bunch of logging settings.  When you run the tests individually, you're running them in a different environment.
> 

This is true, although (a) it rarely seems to matter for me and (b) the environment changes that test-webkitpy does seems to be largely done to work around tests that log more than they should (or log it less controllably). The separate bug that has been open forever about how test-webkitpy configures logging is the right place to debate that, probably.

> This all just seems like an elaborate workaround for the fact that the tests are slow.  If test-webkitpy ran in zero seconds, there would be no reason not to run the whole suite.

I don't think this is quite true; running the tests (or subsets of tests individually) is useful for debugging why things are failing, and is useful for figuring out test coverage, as I mentioned.

But, assuming you can get that functionality easily enough in test-webkitpy (and I don't have any reason to think we couldn't), these differences are just cosmetic.

(In reply to comment #8)
> But perhaps there are arguments like the coverage argument which better support this individual mode.
> 
> I'd like to either:
> 
> - teach webkipy to run them all individually, like you're proposing doing, and auto-add some sort of main
> - remove the ability to run them individually, and instead add all the individual-run features to test-webkitpy

We definitely need to keep the ability to run individual tests. I don't care that much what tool does it, but there needs to be a tool that can run a list of specific test methods and even test modules.

> Having two ways to run the tests seems only to end in sadness (like how it's possible to run them under multiple versions of python does today).

I don't think the analogy is quite the same, but I agree that we should aim for one tool that meets all the needs (or as many as possible). 

Feel free to add the features to test-webkitpy and then delete all of these lines :)