1395630 - Convert config/tests/unit-expandlibs.py to run with 'pytest'

Status: RESOLVED INVALID

(Testing :: Python Test, enhancement)

Description

[Andrew Halberstadt [:ahal]]

These are the only two python unittests still running with the 'unittest' framework. They each had issues when running with pytest, looks like some module import hackery messing stuff up.

Once we get these working with pytest, we can delete all the 'unittest' related code from config/mozunit.py.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

:raphael: would you be interested in looking into this?

Comment 2

[Raphael Aurich [:raphael] UTC+01:00]

:davehunt Sure!

Comment 3

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Add empty test module for pytest conversion;

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

Comment 4

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestRelativize to pytest;

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

Comment 5

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestLibDescriptor to pytest;

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

Comment 6

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandLibsGen to pytest;

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

Comment 7

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandArgs to pytest;

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

Comment 8

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandArgsMore to pytest;

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

Comment 9

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestSectionFinder to pytest;

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

Comment 10

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestSymbolOrder to pytest;

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

Comment 11

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Move converted tests to original test module;

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

Comment 12

[Raphael Aurich [:raphael] UTC+01:00]

Hey :ahal, as you pointed out before, it appears the way we mock out the `expandlibs_config` caused problems when switching the test runner from unittest to pytest. My patch attempts to solve this issue by leveraging pytest fixtures and mock patch for setting config values as opposed to modifying a module variable in test functions.

I broke up my changes in a number of commits, each converting one original unittest test class of `unit-expandlibs.py` to pytest.

Please let me know if you have any questions or would like me to make changes to this patch!

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956466 [details]
Bug 1395630 - Add empty test module for pytest conversion;

https://reviewboard.mozilla.org/r/225366/#review232894

Comment 14

[Andrew Halberstadt [:ahal]]

Thanks for the conversion, I like how you split it up, makes reviewing a lot easier!

That said, this should probably get a build peer stamp of approval before landing. I'll go through and provide feedback, but I'll add your patches to the build review queue so a build peer can grant the final review.

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956467 [details]
Bug 1395630 - Convert TestRelativize to pytest;

https://reviewboard.mozilla.org/r/225368/#review232900

::: config/tests/unit-expandlibs-pytest.py:42
(Diff revision 1)
> +def config_mock(request):
> +    """Replace `expandlibs_config` with a OS specific config."""
> +
> +    m = sys.modules['expandlibs_config'] = imp.new_module('expandlibs_config')
> +
> +    for k, v in request.param.items():
> +        setattr(m, k, v)
> +
> +    config_mocks = [
> +        mock.patch(i, new=m) for i in [
> +            'expandlibs.conf',
> +            'expandlibs_exec.conf',
> +            'expandlibs_gen.conf',
> +        ]
> +    ]
> +
> +    for config_mock in config_mocks:
> +        config_mock.start()
> +
> +    yield m
> +
> +    for config_mock in config_mocks:
> +        config_mock.stop()

It might be helpful (just from a review perspective), if the code this was replacing got deleted from the original test at the same time.

I realize this would temporarily break the original test, but the build peers might want you to collapse this series down to a single commit after the review anyway.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956469 [details]
Bug 1395630 - Convert TestExpandLibsGen to pytest;

https://reviewboard.mozilla.org/r/225372/#review232892

::: config/tests/unit-expandlibs-pytest.py:140
(Diff revision 1)
> [e-mail]
> +def tmpdir():
> +    """Create a temporary directory and remove it after running the tests."""
> +    d = os.path.abspath(tempfile.mkdtemp(dir=os.curdir))
> +    yield d
> +    shutil.rmtree(d)
> +
> +
> [e-mail]
> +def tmppath(tmpdir):
> +    """Return a function to join paths with tmpdir."""
> +
> +    def join(*paths):
> +        """Join the given paths with a temporary directory."""
> +        return os.path.join(tmpdir, *paths)
> +
> +    return join

Pytest has a built-in fixture that can create temporary directories and files:
https://docs.pytest.org/en/latest/tmpdir.html

Might as well re-use that.

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956472 [details]
Bug 1395630 - Convert TestSectionFinder to pytest;

https://reviewboard.mozilla.org/r/225378/#review232904

::: config/tests/unit-expandlibs-pytest.py:595
(Diff revision 1)
> +class FakeProcess(object):
> +    def __init__(self, out, err=''):
> +        self.out = out
> +        self.err = err
> +
> +    def communicate(self):
> +        return (self.out, self.err)

Just repeating the earlier comment, but please remove the code from the old module at the same time as you add it to the new module. This will make it a lot easier to check whether you are simply copying something over, or if you are making any changes.

(this is a general comment, not specifically for this piece of code)

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956473 [details]
Bug 1395630 - Convert TestSymbolOrder to pytest;

https://reviewboard.mozilla.org/r/225380/#review232906

::: config/tests/unit-expandlibs-pytest.py:675
(Diff revision 1)
>              '.text._Z6foobarv',
>              '.text._ZThn4_6foobarv',
>          ]
>  
>  
> +class TestSymbolOrder(object):

This probably applies to most of the other commits too, but let's not use a class if we don't need to. I think this could just be a series of top-level functions.

I also wonder if it's worth splitting this test up into multiple files (and sharing fixtures with conftest.py), but that's a question better left to the build peers.

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8956474 [details]
Bug 1395630 - Move converted tests to original test module;

https://reviewboard.mozilla.org/r/225382/#review232908

Comment 20

[Andrew Halberstadt [:ahal]]

First, sorry for the delayed review, I just got back from PTO. Second, unflagging the review is equivalent to no issues.

This series looks really good overall, I'd be tempted to even let it land as is if I were allowed. But there are a couple themes I'd like to see addressed before passing this on to the build peers:

1) Removing code from the old modules as it gets added to the new one (for review clarity)
2) Using top-level functions whenever classes are unnecessary. If you want, you could format this such that each TestClass in the old module gets a brand new pytest file (with shared fixtures in conftest.py). But might be best to wait on this for now.

Once those are addressed, feel free to re-flag me (or flag the 'build' user on mozreview directly).

Thanks again, this is really good stuff!

Comment 21

[Andrew Halberstadt [:ahal]]

Just confirming this passes for me locally.

Comment 22

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert config mocking to pytest;

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

Comment 23

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestRelativize to pytest;

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

Comment 24

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestLibDescriptor to pytest;

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

Comment 25

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Implement function to get tmp file paths;

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

Comment 26

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert file helper function to pytest fixtures;

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

Comment 27

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Move touch helper function to new module;

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

Comment 28

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandLibsGen to pytest;

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

Comment 29

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Implement assertRelEqual helper function;

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

Comment 30

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandInit setup to pytest fixtures;

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

Comment 31

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert TestExpandArgs to pytest;

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

Comment 32

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_makelist to pytest;

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

Comment 33

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Implement fixture to patch subprocess.call;

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

Comment 34

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Implement fixture to patch subprocess.check_output;

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

Comment 35

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_extract to pytest;

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

Comment 36

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Implement pytest fixture to mock subprocess.Popen;

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

Comment 37

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_getSections to pytest;

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

Comment 38

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_getOrderedSections to pytest;

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

Comment 39

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_getFoldedSections to pytest;

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

Comment 40

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Convert test_getOrderedSectionsWithICF to pytest;

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

Comment 41

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1395630 - Move converted tests to original module;

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

Comment 42

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8956466 [details]
Bug 1395630 - Add empty test module for pytest conversion;

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

Comment 43

[Raphael Aurich [:raphael] UTC+01:00]

Hey ahal,

I edited the commit history so that removed code from the old modules shows up in the same diff as it gets added to the new one and moved the tests functions outside of their respective classes. It appears using pytest's `tmpdir` fixture is problematic as the tests fail if the files are not created in the current working directory. For the sake of clarity, I renamed our fixture to not shadow pytest's fixture.

Please let me know your thoughts!

Comment 44

[Andrew Halberstadt [:ahal]]

Thanks, this much easier to review! Just to warn you, after the review is done, I'm probably going to ask you to fold this all back up since some of the intermediate changes will cause test/build failures :). But for now, this makes it a lot easier to review.

Mike, it looks like you've modified this file the most. I've already looked over these commits and it looks good to me (though I don't have any context around this test). Do you want me to set you as the reviewer, or do I have permission to go ahead and flip these to an r+?

Comment 45

[Mike Hommey [:glandium]]

The file is gone in bug 1429875.

Comment 46

[Andrew Halberstadt [:ahal]]

Doh! Sorry Raphael, looks like we may have wasted a bunch of your time :(. I'm not sure if you were planning on tackling bug 1445273 or not, but this time let's flag the test owner for needinfo before starting just to make sure the test is actually valuable in the first place.