Closed Bug 1302172 Opened 3 years ago Closed 3 years ago

[mozlint] Convert unittest tests to pytest

Categories

(Firefox Build System :: Lint and Formatting, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ahal, Assigned: ahal)

Details

Attachments

(1 file)

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!
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 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)
> +
> +
> +@pytest.fixture(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):
> +@pytest.fixture
> +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)]
> +@pytest.fixture(params=['string.lint', 'regex.lint', 'external.lint'])
> +def linter(request):
> +    return os.path.join(here, 'linters', request.param)

Can depend on lintdir fixture.
Attachment #8790385 - Flags: review?(mjzffr) → review+
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 :)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8bce29dda76e
[mozlint] Convert unittest tests to use pytest instead, r=maja_zf
https://hg.mozilla.org/mozilla-central/rev/8bce29dda76e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Testing → Firefox Build System
You need to log in before you can comment on or make changes to this bug.