Development in the post-Spine project era

[manuelma]

I feel we need to take stronger measures not to break functionality now the Spine project has ended. During the project, we were rushing to get features in to meet our goals, and unfortunately most of the times we didn't provide the necessary unit-tests (I'm looking especially at myself). But we had resources to fix things on the spot and put out fires -- this is no longer the case.

However, I feel development is still very active. So now, it has already happened a couple of times to me that pulling the latest breaks my project, or I find that some things do not work anymore. Again, I have the least moral authority to complain about this as I might be the main responsible for low test coverage.

But maybe it's not too late to do things better. For example, if we're gonna mess with parts of the code for which we don't have unit-tests, from now on we could write those tests before refactoring.

What do you think?

[ghost]

Hi Manuel, thanks for opening the discussion! I was just looking at the test history and indeed there are many commits that have broken the tests. There are couple of ways to mitigate:

1. Always make sure that if you push or merge to master, the test suite passes. If unsure, check test results after push or merge.
2. Make your developments in a separate branch, test, fix, test again. Only merge when everything runs fine. When you push your branch to GitHub it will be tested automatically.
3. Limit the people who are allowed to push and merge to master. Not ideal, but would of course help keep things more organised (I’m not saying we should go into this.)
4. If you see that the tests in master are broken, try to fix asap. Or at least inform or create an issue.

@soininen I know you’ve experience, any comments?

[manuelma]

Thanks @ererkka

What you say about unit-tests is good, but I am talking of many situations where tests don't cut it. Commits that have broken my projects are ones where tests passed. Conversely, many commits where tests fail don't have any undesired effect in the usage of the application, because it's just the tests that fail, and the source code is good.

[ghost]

I see, that’s a valid point as well! Execution test would help here if they covered more of the possible situations.

Another approach is to wind down speed a little bit a think more. All changes should broadly fall into these categories:

1. Bug fixes: needed acutely, should never change or break existing functionality or break projects
2. New features which are backwards compatible, i.e. do not break existing projects. Sometimes acutely needed.
3. New features which (for some reason) require backwards incompatible changes, might break existing projects. Maybe not so acute.
4. Deprecation of some feature. Not acute usually, will break some existing (old) projects.

When making a change (basically doing anything with the Toolbox code), one should think which of the categories their change belongs to. Depending on the nature of the change, one should think what is the best way of implementing those changes. Bugfix? Commit to master. New feature? Make a new branch, test, merge if all is fine. Discuss before making breaking changes.

[soininen]

GitHub action statistics look a lot worse than the situation actually is lately due to some tests failing randomly on timing issue unrelated to working code. I hope I got this issue fixed in a recent commit.

I think the cure for the general situation is what it always has been: improve unit tests. We really need to learn to step back before pushing to GitHub and make sure we've done at least something to cover our changes by tests. Even spend some time to solely improve the tests. As I see it, the alternate route is to force more testing e.g. by pull requests but I'm not sure we have the resources for lots of code reviews.

[DillonJ]

A further thought here is that we seem to be imposing pretty high standards when it comes to acceptance of PRs... so it's not cool if those with push rights just do it willy-nilly :-) (not suggesting that's what's happening)

[ghost]

You mean that if someone really is doing a PR, then it gets a thorough review, but at the same time commits are pushed to master with much lower standards? This really might feel unequal.

[manuelma]

I agree, maybe it's better to slow down a notch, and coordinate resources to review PRs. I remember we wrote names of people who would be in charge of different repos, I was in a couple of them. I feel I might definitely have stuff to say about PRs just by looking at code.

The situation is still quite good in my opinion... but I feel it may get worse if we don't take measures. I also have the feeling that there's some pressure to fix stuff/add features because we are using toolbox in our projects, and we need people to fix and push. But we can use a branch for those situations and delay the merge into master until more people have reviewed the code.