The explanation is inside the patch. We also add the HGSKIPREPO
variable that I forgot to add in the test environment.
Details
Details
- Reviewers
ardumont - Group Reviewers
Reviewers - Commits
- rDLDHG773d872a8164: Move `os.environ` manipulation to pre_cleanup
Diff Detail
Diff Detail
- Repository
- rDLDHG Mercurial loader
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 21361 Build 33179: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 33178: arc lint + arc unit
Event Timeline
Comment Actions
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
Comment Actions
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.
| swh/loader/mercurial/from_disk.py | ||
|---|---|---|
| 171 | do they have a clean environment because we clean it for the tests already? 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 ;) | |
Comment Actions
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 | ||
|---|---|---|
| 171 |
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. [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? | |
Comment Actions
Better fix for environment manipulation
Also fix a typo in the new environment variable. Oops.
| swh/loader/mercurial/from_disk.py | ||
|---|---|---|
| 171 | 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. | |
| swh/loader/mercurial/tests/conftest.py | ||
| 49 | I have a new explanation in the updated commit message. | |
Comment Actions
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.
Comment Actions
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.