Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

set a dummy QISKIT_SETTINGS so tests do not interact with local env #12463

Merged
merged 5 commits into from
Jul 4, 2024
Merged

set a dummy QISKIT_SETTINGS so tests do not interact with local env #12463

merged 5 commits into from
Jul 4, 2024

Conversation

1ucian0
Copy link
Member

@1ucian0 1ucian0 commented May 27, 2024

Some of the tests were failing with in my machine and I noticed that it was because my particular setting in ~/.qiskit/settings.conf. Tests should not depend on the situation of the setting.conf.

This PR sets a dummy QISKIT_SETTINGS to avoid that.

@1ucian0 1ucian0 requested review from nonhermitian and a team as code owners May 27, 2024 12:28
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

@coveralls
Copy link

coveralls commented May 27, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9790432013

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 30 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.04%) to 89.808%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 92.37%
crates/qasm2/src/parse.rs 24 96.23%
Totals Coverage Status
Change from base Build 9785044203: -0.04%
Covered Lines: 65107
Relevant Lines: 72496
💛 - Coveralls

@1ucian0 1ucian0 added the type: qa Issues and PRs that relate to testing and code quality label Jun 12, 2024
@1ucian0 1ucian0 changed the title set a dummy QISKIT_SETTINGS to avoid interfering with local env set a dummy QISKIT_SETTINGS so tests do not interact with local env Jul 3, 2024
1ucian0
1ucian0 commented Jul 3, 2024
View reviewed changes
test/utils/base.py Outdated
Comment on lines 248 to 250
"""Qiskit-specific further additions for test cases, if ``testtools`` is available.

It is not normally safe to derive from this class by name; on import, Terra checks if the
It is not normally safe to derive from this class by name; on import, Qiskit checks if the
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated. Just removing Terra things.

eliarbel
eliarbel previously approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ElePT
ElePT previously approved these changes Jul 4, 2024
View reviewed changes
@1ucian0 1ucian0 dismissed stale reviews from ElePT and eliarbel via 3f26e14 July 4, 2024 07:44
ElePT
ElePT reviewed Jul 4, 2024
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1ucian0, why revert the removal of "Terra"? 3f26e14

ElePT
ElePT approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I see, it's been moved to #12720. LGTM.

@ElePT ElePT enabled auto-merge July 4, 2024 08:12
@ElePT ElePT added this pull request to the merge queue Jul 4, 2024
Merged via the queue into Qiskit:main with commit 63b459e Jul 4, 2024
15 checks passed
@jakelishman
Copy link
Member

Fwiw, this was already handled by tox environment isolation; QISKIT_SETTINGS is not passed on by tox to the runner. There's many environment variables that can influence testing, and it's also not a done deal that doing this at setUp is good enough - Qiskit's "config" module happens to reload the file on basically every check of it (iirc), but a more sane implementation might have loaded the file just once at import qiskit (which happens before this environment patching) and so be unaffected by it.

Tests that can be influenced unduly by options in the settings file should probably be passing the options precisely within the tests to avoid this.

@1ucian0 1ucian0 deleted the tests/mock_setting/1 branch July 4, 2024 13:03
@1ucian0 1ucian0 restored the tests/mock_setting/1 branch July 4, 2024 13:03
@1ucian0 1ucian0 mentioned this pull request Jul 4, 2024
Merged
@ElePT ElePT added the Changelog: None Do not include in changelog label Jul 30, 2024
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
@1ucian0 @Procatv
set a dummy QISKIT_SETTINGS so tests do not interact with local env (Q…
2b0bee4 
…iskit#12463)

* mock QISKIT_SETTINGS to avoid interfering with my own env

* set a dummy QISKIT_SETTINGS to avoid interfering with local env

* apply to all of them

* revert
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElePT ElePT ElePT approved these changes

@eliarbel eliarbel eliarbel left review comments

@nonhermitian nonhermitian Awaiting requested review from nonhermitian

Assignees
No one assigned
Labels
Changelog: None Do not include in changelog type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@1ucian0 @qiskit-bot @coveralls @jakelishman @eliarbel @ElePT