Page MenuHomeSoftware Heritage
Differential D5691

Remove `os.environ` manipulation in tests
ClosedPublic
Actions

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

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
rDLDHG773d872a8164: Move `os.environ` manipulation to pre_cleanup
Summary

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

Diff Detail

Unit TestsFailed
View All

TimeTest
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_from_disk::test_examples
request = <SubRequest 'swh_storage_postgresql' for <Function test_examples>> @pytest.fixture
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_from_disk::test_load_unchanged_repo__dangling_extid
request = <SubRequest 'swh_storage_postgresql' for <Function test_load_unchanged_repo__dangling_extid>> @pytest.fixture
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_from_disk::test_load_unchanged_repo_should_be_uneventful[extid]
request = <SubRequest 'swh_storage_postgresql' for <Function test_load_unchanged_repo_should_be_uneventful[extid]>> @pytest.fixture
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_from_disk::test_load_unchanged_repo_should_be_uneventful[same storage]
request = <SubRequest 'swh_storage_postgresql' for <Function test_load_unchanged_repo_should_be_uneventful[same storage]>> @pytest.fixture
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.mercurial.tests.test_from_disk::test_loader_hg_new_visit_no_release
request = <SubRequest 'swh_storage_postgresql' for <Function test_loader_hg_new_visit_no_release>> @pytest.fixture
View Full Test Results (22 Failed · 25 Passed)

Event Timeline

Alphare created this revision.May 5 2021, 10:33 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMay 5 2021, 10:33 PM
Alphare added a child revision: D5687: Make the Mercurial loader incremental.May 5 2021, 10:34 PM
swh-public-ci added a comment.May 5 2021, 10:34 PM

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!
Alphare updated this revision to Diff 20322.May 5 2021, 10:40 PM

Add pytest-postgres override

swh-public-ci added a comment.May 5 2021, 10:43 PM

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.

Harbormaster completed remote builds in B21304: Diff 20322.May 5 2021, 10:43 PM
Alphare requested review of this revision.May 5 2021, 10:43 PM
ardumont added a subscriber: ardumont.May 6 2021, 9:06 AM
ardumont added inline comments.
swh/loader/mercurial/from_disk.py
172

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
49

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

Alphare updated this revision to Diff 20325.May 6 2021, 10:08 AM

Refactor test detection in a function

swh-public-ci added a comment.May 6 2021, 10:11 AM

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.

Harbormaster completed remote builds in B21307: Diff 20325.May 6 2021, 10:11 AM
ardumont added inline comments.May 7 2021, 9:33 AM
swh/loader/mercurial/from_disk.py
172

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?

Alphare updated this revision to Diff 20382.May 7 2021, 11:58 AM

Better fix for environment manipulation

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

swh/loader/mercurial/from_disk.py
172

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.

swh-public-ci added a comment.May 7 2021, 12:00 PM

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.

Harbormaster completed remote builds in B21361: Diff 20382.May 7 2021, 12:00 PM
ardumont accepted this revision.May 7 2021, 12:06 PM

lgtm

Thanks.

swh/loader/mercurial/from_disk.py
172

\o/

This revision is now accepted and ready to land.May 7 2021, 12:06 PM
Alphare updated this revision to Diff 20389.May 7 2021, 1:26 PM

Remove pytest-postgresql hotfix

swh-public-ci added a comment.May 7 2021, 1:29 PM

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.

Harbormaster completed remote builds in B21368: Diff 20389.May 7 2021, 1:29 PM
Closed by commit rDLDHG773d872a8164: Move `os.environ` manipulation to pre_cleanup (authored by Alphare). · Explain WhyMay 7 2021, 1:33 PM
This revision was automatically updated to reflect the committed changes.
Alphare added a commit: rDLDHG773d872a8164: Move `os.environ` manipulation to pre_cleanup.

Revision Contents
Changeset List

PathSize
swh/
loader/
mercurial/
19 lines
tests/
1 line

Diff 20320

View Options

swh/loader/mercurial/from_disk.py

Loading...
View Options

swh/loader/mercurial/tests/conftest.py

Loading...
About · Developer information · Legal