1302172 - [mozlint] Convert unittest tests to pytest

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Andrew Halberstadt [:ahal]]

I've always heard of the virtues of pytest and have been meaning to try it out. Now that it is in-tree, I decided to try converting the mozlint tests to it as an experiment. It went very smoothly and output looks much nicer than before!

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1302172 - [mozlint] Convert unittest tests to use pytest instead,

Review commit: https://reviewboard.mozilla.org/r/78244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78244/

Comment 2

[Andrew Halberstadt [:ahal]]

Sorry, the diff is a bit tricky to read. But I've tested the tests all still work and do what they're supposed to. I'm more interested in review related to pytest usage.. e.g I might be doing something silly, or not realize a cool pytest feature I should be using.

Comment 3

[Andrew Halberstadt [:ahal]]

Here's the patch on try:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=20547dc808b54962747aa2fef0f7ea9e175e8de4&tochange=28a558023ccaa7f61925cb01c16095c115eeb18d

(second push introduces a failure on purpose to make sure the job still turns red)

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8790385 [details]
Bug 1302172 - [mozlint] Convert unittest tests to use pytest instead,

https://reviewboard.mozilla.org/r/78244/#review77162

Cool! r+wc
I've tried to suggest ways take advantage of pytest features. Nothing major jumped out at me.

::: python/mozlint/test/conftest.py:27
(Diff revision 1)
> +def files(request):
> +    filedir = os.path.join(here, 'files')

You can make the `files` fixture depend on the `filedir` fixture: something like `def files(filedir, request):`

Same for lintdir below.

::: python/mozlint/test/conftest.py:29
(Diff revision 1)
> +
> +
> [e-mail](scope='module')
> +def files(request):
> +    filedir = os.path.join(here, 'files')
> +    suffix_filter = getattr(request.module, 'files', [''])

This doesn't seem to be used in any modules that use this fixture, FYI.

::: python/mozlint/test/test_parser.py:21
(Diff revision 1)
> +def parse():
> +    parser = Parser()
>  
> -    def __init__(self, *args, **kwargs):
> -        TestCase.__init__(self, *args, **kwargs)
> +    def _parse(name):
> +        path = os.path.join(here, 'linters', name)

Another opportunity to use lintdir fixture.

::: python/mozlint/test/test_parser.py:39
(Diff revision 1)
> -        self.assertIn('payload', linter)
> -
> -    def test_parse_invalid_type(self):
> -        with self.assertRaises(LinterParseError):
> -            self.parse('invalid_type.lint')
> -
> -    def test_parse_invalid_extension(self):
> -        with self.assertRaises(LinterParseError):
> -            self.parse('invalid_extension.lnt')
> -
> -    def test_parse_invalid_include_exclude(self):
> -        with self.assertRaises(LinterParseError):
> -            self.parse('invalid_include.lint')
> -
> -        with self.assertRaises(LinterParseError):
> -            self.parse('invalid_exclude.lint')
> +def test_parse_invalid_type(parse):
> +    with pytest.raises(LinterParseError):
> +        parse('invalid_type.lint')
> +
> +
> +def test_parse_invalid_extension(parse):
> +    with pytest.raises(LinterParseError):
> +        parse('invalid_extension.lnt')
> +
> +
> +def test_parse_invalid_include_exclude(parse):
> +    with pytest.raises(LinterParseError):
> +        parse('invalid_include.lint')
> +
> +    with pytest.raises(LinterParseError):
> +        parse('invalid_exclude.lint')

These 4-5 tests could be rolled into 1 parametrized test. (`@pytest.mark.parametrize`), but on the other hand if you keep them separate you have nice, descriptive test names.

::: python/mozlint/test/test_types.py:17
(Diff revision 1)
>  
>  here = os.path.abspath(os.path.dirname(__file__))
>  
>  
> -class TestLinterTypes(TestCase):
> [e-mail]
> +def path():

This fixture can depend on filedir fixture.

::: python/mozlint/test/test_types.py:25
(Diff revision 1)
> -    def __init__(self, *args, **kwargs):
> -        TestCase.__init__(self, *args, **kwargs)
>  
> -        self.lintdir = os.path.join(here, 'linters')
> -        self.filedir = os.path.join(here, 'files')
> -        self.files = [os.path.join(self.filedir, f) for f in os.listdir(self.filedir)]
> [e-mail](params=['string.lint', 'regex.lint', 'external.lint'])
> +def linter(request):
> +    return os.path.join(here, 'linters', request.param)

Can depend on lintdir fixture.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8790385 [details]
Bug 1302172 - [mozlint] Convert unittest tests to use pytest instead,

https://reviewboard.mozilla.org/r/78244/#review77162

> You can make the `files` fixture depend on the `filedir` fixture: something like `def files(filedir, request):`
> 
> Same for lintdir below.

Thanks, yeah I was wondering if there was a way to make fixtures depend on one another.. I should of thought of this :)

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8790385 [details]
Bug 1302172 - [mozlint] Convert unittest tests to use pytest instead,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78244/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bce29dda76e
[mozlint] Convert unittest tests to use pytest instead, r=maja_zf

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bce29dda76e