Page MenuHomeSoftware Heritage

Remove `os.environ` manipulation in tests
ClosedPublic

Authored by Alphare on May 5 2021, 10:33 PM.

Details

Summary

The explanation is inside the patch. We also add the HGSKIPREPO
variable that I forgot to add in the test environment.

Diff Detail

Event Timeline

Build has FAILED

Patch application report for D5691 (id=20320)

Rebasing onto 888471483a...

Current branch diff-target is up to date.
Changes applied before test
commit ecb99e7ddbcb91b51f414c1cda3a36e1d71858d7
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Wed May 5 22:29:27 2021 +0200

    Remove `os.environ` manipulation in tests
    
    The explanation is inside the patch. We also add the `HGSKIPREPO`
    variable that I forgot to add in the test environment.

Link to build: https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/228/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/228/console

Harbormaster returned this revision to the author for changes because remote builds failed.May 5 2021, 10:34 PM
Harbormaster failed remote builds in B21302: Diff 20320!

Add pytest-postgres override

Build is green

Patch application report for D5691 (id=20322)

Rebasing onto 888471483a...

Current branch diff-target is up to date.
Changes applied before test
commit eaa07cc0a3d2995698d855f0f267ea27569ea550
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Wed May 5 22:29:27 2021 +0200

    Remove `os.environ` manipulation in tests
    
    The explanation is inside the patch. We also add the `HGSKIPREPO`
    variable that I forgot to add in the test environment.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/230/ for more details.

ardumont added inline comments.
swh/loader/mercurial/from_disk.py
173

do they have a clean environment because we clean it for the tests already?
(if yes, have you tried to remove that setup ^and check the behavior without the conditional?)

If we absolutely need this, at least, centralize the conditional in a function:

def testing_environment():
    return "PYTEST_CURRENT_TEST" in os.environ
...

# in runtime code
if not testing_environment():
    ...

but if you prefer returning the conditional (make it negative in the function, positive at call time), fine with me as well ;)

swh/loader/mercurial/tests/conftest.py
50

there we go, it is configured for the test ;)

Refactor test detection in a function

Build is green

Patch application report for D5691 (id=20325)

Rebasing onto 888471483a...

Current branch diff-target is up to date.
Changes applied before test
commit 24ab587b6440850951bdcfeeef8a40d7e3eaee9a
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Wed May 5 22:29:27 2021 +0200

    Remove `os.environ` manipulation in tests
    
    The explanation is inside the patch. We also add the `HGSKIPREPO`
    variable that I forgot to add in the test environment.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/231/ for more details.

swh/loader/mercurial/from_disk.py
173

do they have a clean environment because we clean it for the tests already?
(if yes, have you tried to remove that setup ^and check the behavior without the conditional?)

ping?

My trail of thought there is why bypass this in test [1] and not remove the cleaning up done in the test context altogether instead.
Since the main code is now already cleaning up the environment.

[1] opening divergent behavior for test in production code sounds wrong to me.

swh/loader/mercurial/tests/conftest.py
49

Following my last remark above, isn't this redundant with the cleaning up done in the production code already?

Better fix for environment manipulation

Also fix a typo in the new environment variable. Oops.

swh/loader/mercurial/from_disk.py
173

Sorry, I must have misunderstood part of your comment and thought only the code change would suffice.

I've figured out the underlying issue. I had a loader that I was initializing without calling .load(), which caused the environment to be left in a broken state. I'm updating this patch to instead do this in pre_cleanup. The full explanation is in the commit message.
I've also removed the useless loader from the next patch.

swh/loader/mercurial/tests/conftest.py
49

I have a new explanation in the updated commit message.

Build is green

Patch application report for D5691 (id=20382)

Rebasing onto 888471483a...

Current branch diff-target is up to date.
Changes applied before test
commit 4a2541c1b7868574eb772bc372829904f03c37d7
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Wed May 5 22:29:27 2021 +0200

    Move `os.environ` manipulation to pre_cleanup
    
    Simply initializing a loader would empty the environment, which can
    cause seemingly unrelated things to break. Moving the environment
    handling to the `pre_cleanup` phase ensures that `cleanup` will also
    be called and the environment will not be left in a broken state.
    
    We also add the `HGRCSKIPREPO` variable that I forgot to add in the
    test environment. This is still needed because the tests invoke
    `hg` directly. We could potentially have a wrapper util that uses a
    context-manager to do the environment manipulation closer to the issue,
    but we'd have to make sure that no other bare `hg` invocations can
    happen, even in random subprocesses.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/233/ for more details.

lgtm

Thanks.

swh/loader/mercurial/from_disk.py
173

\o/

This revision is now accepted and ready to land.May 7 2021, 12:06 PM

Remove pytest-postgresql hotfix

Build is green

Patch application report for D5691 (id=20389)

Rebasing onto 888471483a...

Current branch diff-target is up to date.
Changes applied before test
commit 773d872a81645505cd7c78e2c4ee819856eaad24
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Wed May 5 22:29:27 2021 +0200

    Move `os.environ` manipulation to pre_cleanup
    
    Simply initializing a loader would empty the environment, which can
    cause seemingly unrelated things to break. Moving the environment
    handling to the `pre_cleanup` phase ensures that `cleanup` will also
    be called and the environment will not be left in a broken state.
    
    We also add the `HGRCSKIPREPO` variable that I forgot to add in the
    test environment. This is still needed because the tests invoke
    `hg` directly. We could potentially have a wrapper util that uses a
    context-manager to do the environment manipulation closer to the issue,
    but we'd have to make sure that no other bare `hg` invocations can
    happen, even in random subprocesses.

See https://jenkins.softwareheritage.org/job/DLDHG/job/tests-on-diff/235/ for more details.