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

factor out function for getting top level directory of cloudinit #1136

Merged
merged 10 commits into from
Dec 8, 2021

Conversation

holmanb
Copy link
Member

@holmanb holmanb commented Dec 7, 2021

Add a test helper to get top level directory

Many tests need to get the location of files & dirs within the cloud-init
project directory.

Tests implement this in various different ways, and often those ways
depend on the current working directory of the pytest invocation.
Create helper functions (and tests) that gets the path of the top
directory or any sub directory under the top directory. This function
does not depend on the environment.

Additional Context

  • This is not a very high value refactor, but not being able to run pytest wherever I please has been bugging me.
  • It should also makes a common task less slightly less error prone and tedious.
  • I'm sure that I missed tests.
  • The tests I converted were unittests that fail when running pytest in a subdirectory.

python_prog = '\n'.join(
[
'import json, sys',
'sys.path.append("{}")'.format(get_top_level_dir()),
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting the pythonpath environment variable in subp didn't seem to work. That would have been cleaner, but this works too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably want to prepend, not append. The right 'cloudinit.subp' to import is the one in the top level dir, if there were others in the path for some reason then you'd get the wrong one.

that said, '' is (I think) usually the first token of sys.path, which is why this worked before. So I don't really think you have to change anything unless the intent is to execute these tests with some other working directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call @smoser. Thanks!

Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I like the overall approach...just some inline questions about the function implementation.

Also, I think test_schema.py, line 53 needs this too. It currently looks like
files = list(Path("../../cloudinit/config/").glob("cc_*.py"))
which doesn't work for me since I'm not running pytest from that directory. I have it replaced locally with

    files = list(
        (
            Path(__file__).parent.parent.parent.parent / "cloudinit" / "config"
        ).glob("cc_*.py")
    )

which is much less nice than using this would be.

tests/unittests/helpers.py Outdated Show resolved Hide resolved
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks!

@TheRealFalcon TheRealFalcon merged commit 65c2cfd into canonical:main Dec 8, 2021
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