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.

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

fix(test): remove env var set by snapped testing programs #564

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

bepri
Copy link
Contributor

@bepri bepri commented Nov 22, 2024

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

Two particular tests would, down the line, run a check against os.getenv("SNAP_INSTANCE_NAME") and default to a known value if it was unset. If these tests were ran directly in a standard ("non-snapped") program, this was fine. However, when running it using a snapped program (in my case, snapped VS Code's integrated testing tools), SNAP_INSTANCE_NAME will be set to the name of that program (again, in my case, code).

Since the known value was never assigned, the test could not know what string to look for, causing this test to fail.

To avoid this failure, this PR removes that potentially confusing environment variable before running the test.

@bepri bepri requested review from lengau and mr-cal November 22, 2024 20:56
@bepri bepri self-assigned this Nov 22, 2024
@bepri bepri force-pushed the work/snapped-vscode-testing branch from 5db55f1 to 2625dd6 Compare November 22, 2024 20:59
Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Nice catch.

Linter failures look unrelated.

Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Oh will you clear SNAP_NAME and CRAFT_SNAP_CHANNEL too? May as well address those too

@bepri
Copy link
Contributor Author

bepri commented Nov 22, 2024

Linter failures look unrelated.

Yeah, I can't seem to even reproduce that mypy warning locally. The line it's accusing is a function that takes no arguments at all, so I think "too few arguments" isn't really possible.

@bepri
Copy link
Contributor Author

bepri commented Nov 25, 2024

Linter error is being addressed here: #566

@lengau lengau force-pushed the work/snapped-vscode-testing branch from cfda1ca to ed518d6 Compare December 4, 2024 21:52
@mr-cal mr-cal merged commit f854aac into main Dec 5, 2024
9 checks passed
@mr-cal mr-cal deleted the work/snapped-vscode-testing branch December 5, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants