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 tests/user-envs-migration.sh #8079

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

Motivation

See each commit for details.

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

I rather know exactly what check failed than get a nice error message.
I'll go to the line if there is a failure, and then see the comment
above.
It was changed from `XDG_DATA_HOME` to `XDG_STATE_HOME` during review,
but it was forgetten to update this test.
The version was based on when this PR was first created; by the time it
was merged the test was dead code as we no longer tested a Nix version
that old.
@Ericson2314 Ericson2314 requested a review from thufschmitt March 20, 2023 18:50
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner March 20, 2023 18:50
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Mar 20, 2023
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks :)

Things like a29abc9 are a fairly easy and frequent source of bugs, I wonder whether we could do something about them

Comment on lines +29 to +31
# The nix profile should point to the new location
[[ -L ~/.nix-profile ]]
[[ $(readlink ~/.nix-profile) == ~/.local/state/nix/profiles/profile ]]
Copy link
Member

Choose a reason for hiding this comment

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

Wait, that section is actually completely broken, I removed this migration from the original PR after some review suggested it 🤔 . So ~/.nix-profile should still point to the original location

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha! Sorry, I should have marked this as draft as I opened it before tests. So I guess we can instead test that the migration doesn't happen until later OK.

Will rework.

(And I realized how I can run the tests incrementally, just define the env var. Whew. That helps.)

@Ericson2314 Ericson2314 marked this pull request as draft March 20, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants