-
Notifications
You must be signed in to change notification settings - Fork 26
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
Comments
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 |
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. |
Thank you, @emilazy. I'm definitely up for helping out. May I set up github actions to run the tests? |
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. |
Ok, I'll get to it :) |
I have created a pull request (#47) with one unit test and all the boilerplate. I'm looking forward to getting to your feedback :) |
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. |
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. |
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. |
@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. |
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?
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. |
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. |
I invited @lulu-berlin to https://github.com/orgs/NixOS/teams/calamares-nixos-extensions/members |
OMG! Thank you, @Lassulus! ❤️ Ok, let's get this thing started :) |
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.
The text was updated successfully, but these errors were encountered: