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

Unit tests for modules/nixos/main.py #44

Open
lulu-berlin opened this issue Jul 24, 2024 · 14 comments
Open

Unit tests for modules/nixos/main.py #44

lulu-berlin opened this issue Jul 24, 2024 · 14 comments

Comments

@lulu-berlin
Copy link
Member

Is my assumption correct that there are no unit tests for modules/nixos/main.py?

I think it would have probably been possible to catch the swapon bug (#34 fixed in #36) with a unit test and also to avoid future regressions.

What do you think? Would it be possible to add a CI (maybe github actions) to this project that would run such tests? I'm willing to make a PR and scaffold some tests if that's welcome.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/unit-tests-in-calamares-nixos-extensions/49587/1

@emilazy
Copy link
Member

emilazy commented Aug 15, 2024

Yes, I think more automated testing would be fantastic if you’re up for helping out. We don’t have as many resources to maintain the graphical installer as we’d like currently.

@lulu-berlin
Copy link
Member Author

Thank you, @emilazy. I'm definitely up for helping out.

May I set up github actions to run the tests?

@emilazy
Copy link
Member

emilazy commented Aug 16, 2024

It’s great to have more people interested in helping out with the installer :)

I’m not sure if any particular permissions on the repository are required to set that stuff up, but @vlinkz and @ElvishJerricco have admin access to it so if you sent a PR with CI machinery I’m sure they’d be happy to review it and do whatever is necessary to get it set up.

@lulu-berlin
Copy link
Member Author

Ok, I'll get to it :)

@lulu-berlin
Copy link
Member Author

I have created a pull request (#47) with one unit test and all the boilerplate.

I'm looking forward to getting to your feedback :)

@vlinkz @ElvishJerricco

@lulu-berlin
Copy link
Member Author

lulu-berlin commented Oct 31, 2024

@vlinkz @emilazy

It's been quite some time since I created this pull request (#47) after asking in this issue whether there was any interest in it and getting positive reactions. @vlinkz, the maintainer, reacted with a heart to @emilazy who said "Yes, I think more automated testing would be fantastic if you’re up for helping out. We don’t have as many resources to maintain the graphical installer as we’d like currently." He even reacted with a 👍 to my PR.

I tried to make my PR as small as possible and even offered to add documentation to explain the testing strategy.

And... crickets.

I'm just saying, friends: this is quite off-putting. If you don't want contributions, just say so. Don't pretend you're interested in a contribution and then ghost the contributor.

@emilazy
Copy link
Member

emilazy commented Oct 31, 2024

I’m very sorry – I’m a Nixpkgs committer but I don’t personally have rights to merge changes into this repository. I believe @vlinkz and @ElvishJerricco are the only people active in the project who do. I’ve been concerned about the maintenance state of the installer for some time, given how the long process to deal with recent security issues went – for a good while I was considering putting out a call for more maintainers lest we find ourselves forced to remove the graphical installer entirely. I think it’d be great to have more testing and great to have you involved, and I wish I could do more than ping the relevant people about it.

@vlinkz
Copy link
Member

vlinkz commented Oct 31, 2024

I do apologize I really haven't had as much time to dedicate to this as I would like recently. I will try to be better about it and be more responsive. That being said, if you or anyone else is interested in having maintainer rights I would be more than happy to expand the group, and think it would be useful for situations where I'm unavailable. @emilazy I know you already have a bunch of responsibilities in nixpkgs so I don't want to add another one to you with this, but I could add you so you can unstuck things in a pinch.

@lulu-berlin
Copy link
Member Author

@vlinkz, Thank you for merging my PR. 🙏 ❤️

I'm sorry for venting out. I felt like I wasn't able to get any response and I got frustrated. I would really like us to find a friendly way to make sure we can continuously improve QA without having PRs getting stuck in code review.

I would very much like to continue with testing. My PR was just about setting up the groundwork for writing tests and now I think it would make sense to start testing some relevant combinations of user input to avoid regressions.

I don't know if you were thinking about me or only @emilazy when you said "if you or anyone else is interested in having maintainer rights I would be more than happy to expand the group". I'd be happy to join in.

One thing I could work on that could be interesting to try besides unit-testing is to have at least one basic integration test, but for that I might need some more guidance in how to actually deploy and run the graphic installer.

@vlinkz
Copy link
Member

vlinkz commented Nov 3, 2024

I don't know if you were thinking about me or only @emilazy when you said "if you or anyone else is interested in having maintainer rights I would be more than happy to expand the group". I'd be happy to join in.

Sure, I'm happy to add you! Although it doesn't seem like I have permissions to add someone outside the NixOS organization, @emilazy would you have those perms?

One thing I could work on that could be interesting to try besides unit-testing is to have at least one basic integration test, but for that I might need some more guidance in how to actually deploy and run the graphic installer.

This would probably look like a nixos test within nixpkgs, which would be really helpful! It would be run on every new nixos evaluation on hydra and could catch bugs in upgrade and such.

@emilazy
Copy link
Member

emilazy commented Nov 3, 2024

Hi there, sorry for the slow reply. Didn’t mean to call you out here @vlinkz – I totally get not having the time to focus on this stuff and just want to make sure we can move in a direction of getting the maintenance of the installer more sustainable. I truly don’t know anything about Calamares and I feel that Calamares knowledge is the biggest blocker here, so I’d like to resist the temptation to take on responsibility here for the time being. I think it would be great if we could empower people with the interest and skills to work on the installer, so I’m all in favour of onboarding @lulu-berlin. Having integration tests for the installer in NixOS especially would let us be much more proactive in merging improvements without worrying about regressions.

Sadly I don’t have the rights to add people to this repository myself; I believe that has to be done by the GitHub organization owners. I’ll link this thread in the Matrix helpdesk room for them to take a look at.

@Lassulus
Copy link
Member

Lassulus commented Nov 3, 2024

I invited @lulu-berlin to https://github.com/orgs/NixOS/teams/calamares-nixos-extensions/members
That should give the necessary permissions to maintain things in this repo :)

@lulu-berlin
Copy link
Member Author

OMG! Thank you, @Lassulus! ❤️

Ok, let's get this thing started :)

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

No branches or pull requests

5 participants