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

stages: add new unit test for kickstart stage #1425

Merged
merged 3 commits into from
Nov 7, 2023
Merged

stages: add new unit test for kickstart stage #1425

merged 3 commits into from
Nov 7, 2023

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 3, 2023

This commit adds a simple and lightweight unit test for the new kickstart options. It's pretty trivial but also cheap and runs fast. The idea behind it is that the code of the kickstart stage is tested without the need for a full osbuild tree, just passing the test input options and observing the expected file content. The same pattern could potentially be used for other stages (like the user stage) to check that the content of the output has the expected format (or that options that are mutually exclusive are handled correctly).

While this is trivial now and looks a bit like a waste of time I think this will become useful as more kickstart stage options get implemented. The goal is to eventually support the options in https://github.com/RedHatInsights/edge-api/blob/main/templates/templateKickstart.ks in a declarative manner and part of this will involve supporting more things that kickstart is currently doing, e.g https://github.com/RedHatInsights/edge-api/blob/main/templates/templateKickstart.ks#L77 and here we will need to be careful about things like input validation/escaping so having this unit tested seems quite useful. I personally also like it because it tells me at a glance what the kickstart file should look like (test as docuementation).

Note that this is not urgent at all and given how trivial it is I'm fine with being told that this is over-testing. We could keep it in draft stage (or close it even) until there is a more complex use case (as stated above I anticipate there will be one for kickstart).

I'm not happy with chage chage in line 22 (tox.ini), it's needed because with the "*" glob pylint will consider the test dir a python module but because it has no init.py that fails. Adding a init.py will ake pytest fail. The alternative would be to move this under the top level test/ dir (e.g. under test/unit/stages or something.

Having said all this - I would love your intput and feedback :)

@mvo5 mvo5 requested a review from achilleas-k November 3, 2023 14:59
@mvo5 mvo5 mentioned this pull request Nov 3, 2023
Merged
supakeen
supakeen reviewed Nov 6, 2023
View reviewed changes
stages/test/org_osbuild_kickstart.py Outdated Show resolved Hide resolved
stages/test/test_kickstart.py Outdated Show resolved Hide resolved
stages/test/test_kickstart.py Outdated Show resolved Hide resolved
stages/test/test_kickstart.py Outdated Show resolved Hide resolved
@supakeen
Copy link
Member

supakeen commented Nov 6, 2023

This is neither a waste of time nor trivial. I'd love to have a quicker way to test stages doing the right things :)

@supakeen
Copy link
Member

supakeen commented Nov 6, 2023

I'm not happy with chage chage in line 22 (tox.ini), it's needed because with the "*" glob pylint will consider the test dir a python module but because it has no init.py that fails. Adding a init.py will ake pytest fail. The alternative would be to move this under the top level test/ dir (e.g. under test/unit/stages or something.

I like the stage tests living close to the stages. People do write their own stages as well and it fits better in future (tm) plans to support different types of stages which each have their own test.

@mvo5 mvo5 force-pushed the kickstart-stage-test branch 2 times, most recently from d05a5e6 to cbaa24e Compare November 6, 2023 10:55
mvo5
mvo5 commented Nov 6, 2023
View reviewed changes
stages/test/test_kickstart.py Outdated Show resolved Hide resolved
@mvo5 mvo5 requested a review from supakeen November 6, 2023 10:56
@mvo5 mvo5 marked this pull request as ready for review November 6, 2023 10:56
@mvo5
Copy link
Contributor Author

mvo5 commented Nov 6, 2023

I'm not happy with chage chage in line 22 (tox.ini), it's needed because with the "*" glob pylint will consider the test dir a python module but because it has no init.py that fails. Adding a init.py will ake pytest fail. The alternative would be to move this under the top level test/ dir (e.g. under test/unit/stages or something.

I like the stage tests living close to the stages. People do write their own stages as well and it fits better in future (tm) plans to support different types of stages which each have their own test.

Thanks, for your excellent feedback! I addressed the points and moved the PR from draft to real and tried to polish it. I'm sure there are a bunch of fixes needed around exact naming and location of the files etc but I hope the shape is now looking good. Fwiw, I'm happy to split the import helper out but given that it's small and the test is the use-case I felt like it might be a good idea to just leave it in :)

supakeen
supakeen previously approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Love it. Let's keep it out of CI for now so we can work with it locally for a bit then enable it later or do we want it in now? (cc: @achilleas-k).

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 6, 2023 •
edited
Loading

Love it. Let's keep it out of CI for now so we can work with it locally for a bit then enable it later or do we want it in now? (cc: @achilleas-k).

Thank you for the review and approval!

I'm not sure I understand what keep it out of CI means? Maybe I'm misunderstanding but I would love to see it merged so that I can add test cases to PR#1426 :)

Silly me, I understand now what you mean and add the new test to the CI runs.

@achilleas-k
Copy link
Member

Does putting a test/ directory under stages/ affect packaging? I think our spec file packages everything under that directory. Do we want these new stage tests distributed in the rpms??

@mvo5
Copy link
Contributor Author

mvo5 commented Nov 6, 2023

Does putting a test/ directory under stages/ affect packaging? I think our spec file packages everything under that directory. Do we want these new stage tests distributed in the rpms??

Thanks, excellent catch (that would have been slightly embarrassing to release with that)! I updated the spec file now to exclude anything test_*.py. Or if you feel the amount of workarounds get unwieldy we could also move into a separate test/stages/unit dir (but of course the downside of that is that tests/code are no longer as close as before).

mvo5 and others added 3 commits November 7, 2023 07:40
@mvo5 @supakeen
osbuild: add new testutil.imports module to help test stages
8848edd 
This commit adds `osbuild.testutil.imports.import_module_from_path`
that can be used to import arbitrary python source files. This
allows importing files from the stages directory that have a
non python friendly filename like `org.osbuild.kickstart`.
@mvo5 @supakeen
stages: add new unit test for kickstart stage
0ba0b30 
This commit adds a simple and lightweight unit test for the new
kickstart options. It's pretty simple but also cheap and runs
fast.
@mvo5 @supakeen
github(workflows): add stages/test to the CI run too
35c231b 
Include the new `stages/test` test category to the CI runs as well.

Note that because `stages/__init__.py` and `stages/test/__init__.py`
are missing it is not possible to use the existing style of just
doing `stages.test`. Adding `stages/__init__.py` feels wrong and
the desire is to have the stages tests close to the stages so this
seems the least invasive way.
supakeen
supakeen approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@supakeen supakeen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very nice, can't wait to expand on this in followups :)

achilleas-k
achilleas-k approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

LGTM

@supakeen supakeen enabled auto-merge (rebase) November 7, 2023 12:22
@supakeen supakeen merged commit 78238ba into osbuild:main Nov 7, 2023
75 checks passed
@mvo5 mvo5 mentioned this pull request Nov 8, 2023
Closed
@mvo5 mvo5 deleted the kickstart-stage-test branch November 9, 2023 11:35
@supakeen supakeen mentioned this pull request Dec 4, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achilleas-k achilleas-k achilleas-k approved these changes

@supakeen supakeen supakeen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mvo5 @supakeen @achilleas-k