Introduce Tests into ProtonUp-Qt

[sonic2kk]

Is your feature request related to a problem? Please describe.
Not really 😛

Describe the solution you'd like
ProtonUp-Qt has become a very mature project in terms of functionality. The codebase is clean and readable imo and I frequently use it as reference when working on my own projects.

As the project is no longer in early stages of development, it might worth spending some time in investigating some unit tests. There has been discussion around re-organizing parts of the code (such as moving some of the util functions around, and restructing ctmods a little). Having a test framework in place could help reduce the risk of regressions during refactors.

Unit tests can also generally be a good way to "check your work", as sometimes when you're writing test cases, it helps you realise something might not be architected in the best way, or you might've taken the wrong approach. (EDIT: I realised when reading this back how this may sound, this is not directed at you or other contributors; it's actually directed a little at myself!)

Using unit tests could also help us catch AppImage compatibility breakages I think, if they're integrated as a workflow that runs on a specific Python instance (if that's not going to be expensive and cost you money 😅).

Of course, unit tests are only as good as how they're written and used. Whether or not unit tests are enforced/encouraged as part of a PR, reviewing tests, it is more of a maintenance burden, but it can help. Also, it's important to be vigilant and not write pointless tests. Not everything needs a test, and 100% code coverage need not be the goal, especially for a GUI program.

Which leads me to: ProtonUp-Qt is heavily Qt-based, and writing UI tests may not be worth anyone's time. What I'm more interested in is having tests for utility functions primarily.

Describe alternatives you've considered
Unit tests are, plainly speaking, a pain in the ass sometimes! Writing a new feature and then thinking "jeez, how am I gonna write a test case for that..." especially for complex cases that require mocking. Simply not bothering is an alternative. There's no burning need, this is really just a bit of spit-and-shine for project polish.

This also won't really have any direct user-facing changes. As such, I can entirely see and understand the mindset of, well: who cares? - This is entirely a wishlist-y proposal, but one that I think long-term could help keep with maintenance and housekeeping.

Does ProtonUp-Qt need tests? In my opinion, it doesn't. But, I could see some cases where they could be helpful. I have caused regressions before, and I should've absolutely been more careful, but having some tests might've helped me catch them.

Additional context
ProtonUp-Qt is a popular project, a mature project, but also a hobby project. I totally understand that unit tests might be way more hassle than they're worth. 😄 Some people get really preachy about tests, and I am not trying to be that way at all. I'm not going to pretend like the project is going to fall apart because of a lack of tests. This is just an idea I wanted to bring up, as I think ProtonUp-Qt is at the stage now where beginning to write tests could be helpful for maintenance (emphasis: could).

---

Implementing tests doesn't have to be an all-at-once endeavour. I'm opening this issue to get a discussion going on the following:

• Are you interested in having tests in ProtonUp-Qt?
• If so, there are some unit test frameworks. Python's built-in unittest is popular but a little ugly, pytest seems to be the most "elegant", but of course this would need discussion. We should also be "careful" of introducing a new dependency if we go with a library like pytest (another alternative is nose2, but I'm not sure how popular that is these days, I'm totally out of the loop!)
• If the above are resolved, where should we start? And starting can be as small as writing one test case for one function. 🙂

I'm a little busy right now, but I'm interested in helping out with this too of course where I can! I write this from the perspective of someone who writes tests for his day job and then maintains a project without any tests. Making large refactors without tests to fall back on is... Scary... 😅

Hope I made sense. Thanks!

[DavidoTek]

Very good point to bring up.

I have though about doing it some while ago but didn't think any further of it.

Using unit tests could also help us catch AppImage compatibility breakages I think,

We might actually want to do it. I'm wondering what would be the best way, if they're integrated as a workflow that runs on a specific Python instance
It might be interesting to do some sort of smoke test in the CI pipeline (at least when doing a new release), as this would show whether the app is actually starting, or whether it crashes with exit code != 0. It is probably a bit more time consuming as we need to setup a X11/Wayland server in the VM for this.

if that's not going to be expensive and cost you money 😅

GitHub gives you 2000 minutes (or 3000 minutes for Pro users) for free every month. Even if there are 100 commits every month, that would allow for a runtime of 20-30 minutes.
Doing a CI run for every commit on every branch would probably be too much then, but doing it on main should be fine. Also, I think GitHub allows to trigger a run when clicking the "merge" button in a PR, so there won't be any unneccesary rollbacks/fixing PRs.

Not everything needs a test, and 100% code coverage need not be the goal, especially for a GUI program.

What we can easily test are utility functions that don't have any side effects (no UI interaction, no filesystem access, no downloads, ...).
I guess it would also be interesting to test functions that download and parse some data (e.g. AWACY) and see if they still work or if the data format has changed. Would be more of a component test than a unit test I guess.

Which leads me to: ProtonUp-Qt is heavily Qt-based, and writing UI tests may not be worth anyone's time. What I'm more interested in is having tests for utility functions primarily.

I agree. UI tests are quite hard to do. I'm not even sure if there's a good testing framework for Qt (something like Selenium for web apps). Also, most of the time, UI won't completly break the app, only some limited functionality.

who cares? - This is entirely a wishlist-y proposal, but one that I think long-term could help keep with maintenance and housekeeping.

Right. Having a working (and maintainable) app is quite important for UX as in that users don't want to get automatic updates that are just plain broken and can't be easily reverted (AppImage is fine, but for Flatpak you need the Commit hash and run a command. Explain that to a non-computer science person 😄).

Does ProtonUp-Qt need tests? In my opinion, it doesn't. But, I could see some cases where they could be helpful. I have caused regressions before, and I should've absolutely been more careful, but having some tests might've helped me catch them.

That is an important aspect. It is very easy to miss even an obvious bug in the code. That happens to me even after doing a code review and running the app.

---

Are you interested in having tests in ProtonUp-Qt?

In general, yes.

If so, there are some unit test frameworks

I haven't done any unit testing with Python yet. I'm open on what framework we could use. I'll read up on that topic.

If the above are resolved, where should we start? And starting can be as small as writing one test case for one function. 🙂

I think that is the way to go.
Trying to do it all at once would be very time consuming and will probably end in some mess and maybe even broken code.
Doing it in an iterative way, slowly increasing test coverage for the existing code and somewhat enforcing unit tests for new code, is probably the best.

I'm a little busy right now, but I'm interested in helping out with this too of course where I can! I write this from the perspective of someone who writes tests for his day job and then maintains a project without any tests. Making large refactors without tests to fall back on is... Scary... 😅

Okay. I hope I will find time in the next days to start working on it. I'll let you know.
I've actually heard that multiple times, that people do tests at work, but don't want to spend their valuable free time to do so for their hoppy project.
But as you have pointed out, ProtonUp-Qt has grown quite large in the last years, so it is probably time to put it to the test 😄

Thanks!

[sonic2kk]

I'm glad this has been well received, I was worried it could come off almost as proposing a solution to a problem that doesn't exist, or needlessly trying to pick fault.

GitHub gives you 2000 minutes (or 3000 minutes for Pro users) for free every month. Even if there are 100 commits every month, that would allow for a runtime of 20-30 minutes.

This is far more generous than I thought, and great news! I think that should be plenty for this project's purposes. Only running for the main branch is sensible and should keep us within this limit.

I guess it would also be interesting to test functions that download and parse some data (e.g. AWACY) and see if they still work or if the data format has changed. Would be more of a component test than a unit test I guess.

Still a great use-case, it can help us know if something regressed in the codebase or if something broke upstream. There was a brief period where the AWACY format was incorrect and ProtonUp-Qt couldn't parse it (fixed with AreWeAntiCheatYet/AreWeAntiCheatYet#1469), I can't remember how I noticed this but some tests would help us diagnose it. Maybe not in CI but when running tests locally we could detect this in a more uniform way.

I haven't done any unit testing with Python yet. I'm open on what framework we could use. I'll read up on that topic.

It's still very new to me as well, I don't have a particularly strong strong opinion either way, but anecdotally pytest seems to be a popular choice. I'm unsure of what advantages it has over the builtin unittest though.

I also don't know the best way to structure tests with any of the frameworks, but getting the ball rolling will be a good first step. Once the way to structure tests is figured out we'll have a good base to build off of. From what I've seen it seems like you import the parts of the code you want to test, but if those functions and methods rely on other functions and methods that we aren't testing we'll probably have to mock some behaviour.

Any resources you find for testing though feel free to post them here for me or others who want to contribute tests! 😄

[DavidoTek]

It's still very new to me as well, I don't have a particularly strong strong opinion either way, but anecdotally pytest seems to be a popular choice. I'm unsure of what advantages it has over the builtin unittest though.

I took a look at it. Pytest seems to be simpler as it doesn't require as much boilerplate code. Also, the tool itself seems to be more flexible. It adds another dependency, but it is only needed for development and the CI.
I guess we can go with it.

https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html

I wonder which folder structure we should choose. I feel like adding another top level directory makes more sense because we don't need to ship tests with the final builds. This also keeps the testing code separated.

https://tox.wiki/en/stable/index.html
checking your package builds and installs correctly under different environments (such as different Python implementations, versions or installation dependencies),

Found this along the way. I guess it would make sense to run the tests on the expected Python version(s). That would also eliminate the problem that we push incompatible code on main.

[sonic2kk]

Apparently, GitHub Actions are free for public repos that use GitHub runners (which in this case I assume we would). ShellCheck CI was recently added to SteamTinkerLaunch and I noticed when checking my used minutes that none were used. I stumbled across these docs: https://docs.github.com/en/billing/managing-billing-for-github-actions/about-billing-for-github-actions

GitHub Actions usage is free for standard GitHub-hosted runners in public repositories

This should ease the restrictions on us for running tests on CI.

This may also open up the possibility to create a workflow that builds an AppImage on each commit, making it easier for users to test if PRs fix given issues. This should be separate from the current AppImage Actions, so as not to create clutter. But that should probably be a separate discussion, just something I wanted to bring up.

[DavidoTek]

GitHub Actions usage is free for standard GitHub-hosted runners in public repositories

Ah, that's great.
Fully checking each PR would also simplify the review process. That ensures that it doesn't break Python version compatibility and internal APIs don't break if we introduce Unit tests.