Can releases please be properly tested?

[thecodingdude]

So this has been on my mind for a few months now, and frustratingly I can't find the link but I recall Taylor saying a few months back there would be no breaking changes in each update to Laravel 5.5 - although I've also seen in the past that there could be breaking changes (and that Laravel doesn't use semver in the strict sense).

Right now we have:

#22830: Collection::get() is broken after commit #22554
#22808: "actingAs" helper is broken in unit tests
#22829: Individual Illuminate packages are not updated since 5.5.28
#6197e56 - revert breaking change

5.5.31: Reverted #22804 (d8a8368, f34926c, #22817)
5.5.30: Reverted #22649 (#22815)
5.5.27: Reverted performance improvements of Model::getTable() (#22478)
5.5.20: Reverted changes to BadMethodException in #20196 (#21929)
5.5.6: Reverted stable sort support in Collection::sortBy() (#21255)

I thought the idea of unit tests were so that things that break can be caught so they don't end up being released, yet with the breakneck speed of how Taylor is releasing new changes (twice in a single day, with a single revert change), and accepting PR's, it doesn't seem like QA is top priority here.

Now, I'm not here to simply call people out, but rather, maybe it'd be worth having a RC period, similar to Linux, or having a 'current', 'beta' or whatever to test new changes, revert them if they're bad, before they make it to release.

I like Laravel, but since 5.5 is an LTS (which isn't made clear from reading the docs, that's another issue in itself), I'd like to see focus on changes that are non-breaking and simply bug fixes, other changes should be merged into 5.6.

I don't really understand why things are being added to 5.5 and not part of the 5.6 branch, especially breaking changes that are seemingly merged and reverted at will.

Cheers.

[browner12]

I agree that it's good to start a discussion about this.

One issue I see is that unit tests are passing when code is being released. Is the issue that we need more unit tests, or are some of these issues outside the scope of unit tests, and we need another layer of testing?

[mnabialek]

I can agree. However making tests better is not always accepted. For example in many tests there are mocks used but in fact they are wrong. For example in this PR #22770 I've tried to fix part of them and it wasn't approved. Having such tests makes false-positive feeling that we have tests and everything is working fine, but in fact it's not - making small change/fix in code might introduce bug but tests won't fail.

Of course it's impossible to have tests that will catch all future problems but for sure they should be improved and coverage should also get higher and when introducing new things tests should be added and at the moment that's not always the case.

Also I don't think we should add all features always to next branch - if we did, we would always had 5.5 with a few releases with bug fixes and everyone had to wait too long for some other useful things.

[thecodingdude]

or are some of these issues outside the scope of unit tests, and we need another layer of testing?

Yes - alphas/betas/release candidates, doesn't matter what they're called, a period of time where the changes are tested before they land in stable, maybe 1/2 weeks at least, with more emphasis placed on unit testing and CI.

When your release cycle is:

Make PR
Taylor merges PR
Release

Then something is seriously wrong. No changes should ever make it to release without being thoroughly and properly tested, and the fact they're being reverted so often obvious indicates a process problem which needs addressing.

Also I don't think we should add all features always to next branch - if we did, we would always had 5.5 with a few releases with bug fixes and everyone had to wait too long for some other useful things.

Pure nonsense and coming from a developer that really disappoints me - NodeJS, VSCode, Electron et al ALL take time before they release, whether that's a nightly version, or insiders branch, or alpha testing, that is what you expect from competent software developers.

If you want the latest features then go use the 5.6 alpha branch, where random breakages are accepted, but for an LTS or any release that has merged to stable you should NEVER break 'userspace', or in this case, cause regression bugs.

The main concern is how Taylor is merging PR's and only reverting them once the community says "this is broke", which should never happen. It's a process problem and the easiest fix is to simply not merge PR's that are not properly tested. How on earth does this get merged, and 19 days later we discover it's a breaking change, what's going on here?

Seems like testing is an afterthought here, and it's causing serious problems. When I update my framework, I don't expect random things to break.

/rant

[browner12]

my concern with "alpha/beta/release" is that now we are relying on users to test the new code, which could lead to inconsistent coverage. for example, for 1 release 50 users have a little extra time at work and decide to run the new beta release against their codebase, but for a second release only 5 people are able to.

is there a better solution for this? or is this a situation where something is better than nothing?

[sisve]

So, we have a few issues. This is my view on the state of things;

1. We need something like a gitflow workflow to have stable releases.
2. We need dedicated release managers for every release. It would be up to the 5.5 Release Manager to merge bugfixes from the develop branch into the 5.5 release. This person should not be Taylor due to (presumed) time constraints.

Regarding the amount of users that tests new things; that's not something that should hurt existing releases. Don't merge badly tested code to have it tested by live environments. I also believe a huge problem is that we accept pull requests without new tests.

Can we setup Travis to calculate code coverage? That, with proper @covers annotations, could very quickly show which components are lacking tests.

[thecodingdude]

@browner12 what I've thought of is this:

ALL PR's are rested for 14 days - no PR's younger than this are ever merged unless it is a really trivial change

PR's are then merged to a 'staging' branch, which is the next release people can test for 1-2 weeks

After that, it's then released to stable.

Aside from the obvious improvement in testing itself, giving more time between PR created and PR merged would be a huge improvement - a regression happening is acceptable, but the amount we're seeing here is not great.

Ideally, a series of policies would be created about what changes would be accepted in a stable release. IMO 5.5 should not be adding new changes or changing things aside from very obvious bug fixes - every single line of code modified is a potential breakage, and you are walking on eggshells in software development. But even if things are added, at least scrutinising them more would be a good start. For example, in #22554 not deleting 4 lines of code in Collection::get and having Taylor merging it within 7 hours, which is just crazy fast and absolutely no regard is given to potential problems.

[browner12]

let me see if I understand correctly.

so you would PR it to a 'staging' branch and a maintainer would merge it in there. then after 1-2 weeks of userland testing, a maintainer (or release manager) would merge that into the appropriate branch?

1 question I have is how do you account for all the rolling changes in staging, because presumably new changes would be coming in every day. A userland tester now has this moving target that they're trying to test against. Would it be difficult to determine what change caused your local tests to fail? Also, when the time came to merge the change in the release branch, how would that specific code be pulled out of Staging and merged into 5.5, for example?

[thecodingdude]

@browner12 IMO I like the way NodeJS handles their releases, it may not be perfect. Take this random issue - it's reviewed by 4 people so far, it has a CI and tells you which tests failed, it's approved, and then it gets merged into the relevant branch.

They have a properly documented contribution guide which goes in depth quite extensively, and they have strict requirements when dealing with any stable branches. They also document their release process including the use of a 'staging branch'. But they are also firm in what changes are allowed - major changes are always pushed to the next release, and backports are always considered/discussed before they land on that branch. As somebody who also uses Node, I've yet to upgrade and notice any issues, and very rarely are things reverted as they are here.

I agree with @sisve here - we need dedicated people to handle the releases if Taylor doesn't have the time himself. It's understandable regressions may happen, but with Travis we can see code coverage, where tests can be improved, taking more time over PR's, and ensuring no breakages happen to the best of our ability would be major improvements.

The current model is too fast - 7 hours is ludicrous speed to accept a PR then land it on master.

[browner12]

definitely +1 on a more extensive Contribution Guide. I think that would help with a lot of problems, and give us much better code consistency throughout the FW.

[Harry-Torry]

You could take X amount of well tested projects and programatically clone/configure/run tests, and ensure they pass. Then upgrade the laravel version and re-run the tests, if there are any differences then you would immediately know of any breaking changes.

[deleugpn]

Feels like you guys are saying you no longer wants a benevolent dictator maintaining Laravel. Discussing the alternatives only makes sense after figuring out whether Taylor drops the BDFL status.

[thecodingdude]

@deleugpn I am absolutely not suggesting that at all. It's entirely possible to be a BDFL and not release broken code into production - Linus Torvalds does a very good job maintaining the Kernel. I think the main issue is there is absolutely no release process whatsoever. Releases just happen at random, there are no reviewers, no announcements, no testing branches...It's just release and see what sticks, and revert anything that's bad.

For a framework as mature as Laravel and as a developer a basic expectation is breaking changes never land in stable, and a proper process similar to Node/VSCode/Electron may help with that. Taylor can still be in charge, and I am not doubting his software development skills but the process under which PR's are merged and releases are cut with bugs.

Linux has a network of trust, people who Linus trusts to submit good patches, such as Greg Kroah-Hartman and will always accept patches from such people. He also gets very angry when rubbish gets thrown his way.

It's all about ensuring what you're pushing is well tested, understood and of a very high quality, not simply accepting a PR 7 hours after it's posted with no testing/feedback, and cutting a release unannounced and untested.

This is extremely basic software development principles that are not being applied, and it gets tiring after a while...

[browner12]

i don't think that's necessarily true. we are just trying to make the process more consistent, and easier for Taylor, since we know their 2 person team definitely has time constraints.

[deleugpn]

Maybe I misread it then. Regardless of the process as a whole, the thing in mind would be to have some sort of develop branch where Taylor can do his thing as usual (nothing changes), but a new process takes care of moving things from develop all the way up to stable branch and, preferably, does not involve Taylor just to save him time?

[Dylan-DPC-zz]

Here are few points from my side:

1. We need to know the roadmap. Just having "no plans of adding this" isn't going to help
2. PRs don't have to be closed in such a hurry. It is better that we give time to contributors to fix issues, provide descriptions and tests, and make changes and also gain more reviews from community.
3. All PRs should go to a "dev" branch (master) and not a release branch (even though it isn't breaking) unless it is fixing a bug.
4. A detailed contributions guideline
5. Ensuring there is consistency in merging PRs. Some are closed for the same the reason that other PRs are merged.
6. More activity in internals channels and other places where proposals can be made & discussed.

[browner12]

1. Is there a roadmap?
2. I understand why he tries to close so quick, because it can quickly become an unmanageable nightmare.
3. no opinion.
4. Definitely need this. Would be willing to help, but I think this needs to be a top-down decision.
5. Ideal, but tough.
6. Definitely need this, too.

[deleugpn]

1. BDFL
2. BDFL
3. Point of Discussion?
4. No opinion
5. BDFL
6. When there are interesting things to be discussed, the PR usually stays open for a longer time. A lot of discussion in internals are nice, but only up to a certain point. It's moot for everyone to discuss a topic if Taylor won't merge it anyway.

[FatBoyXPC]

@sisve makes solid points about testing. Allowing new code without tests is obviously bad, and we obviously need better coverage, too.

In the case of the Arr::get() - that revert should be immediately followed up with a test proving the case that broke.

[Dylan-DPC-zz]

@browner12 keeping prs open for long isn't a nightmare as people think it is.

[sisve]

Here's how things can go wrong;

1. The Arr::get()-from-Collection::get() issue is merged.
2. There's different kind of problems in it.
3. A commit called "revert breaking change" is introduced that just checked for null keys. It didn't actually revert anything, we still call Arr::get().
4. There's now a followup PR for null keys that fixes what the commit didnt. [5.5] Fix Collection::get() on null key and specified default #22837

We've still not reverted the change, we still call Arr::get(), and we still have problem that @jeroennoten noted with nested arrays.

(new Collection(['a' => ['b' => 'c']]))->get('a.b', 'd')

Okay, here's another one.

1. Something affecting Collection::only() is merged with tests. [5.5] Allow only to accept collection of keys #22804
2. A commit called "fix bugs" is introduced that removes the tests, but not the code change.
3. This is released as 5.5.31
4. This is documented as "Fixed" and "Reverted [5.5] Allow only to accept collection of keys #22804" in the release.

So, we left the code that caused the problem, introduced new code, and removed the test. This happens; I've done weird reverts myself in #22532 and #22533.

I believe that these problems occur due to time constraints. Things are done fast, but incorrect. I believe that a dedicated release manager would have proofread the changes, and raised some flags before these commits where released.

We cannot leave code changes, release management and day-to-day project stuff to one individual. There's only 24 hours per day. Offload these tasks to other people, and give us a public roadmap so we know where we're heading.

[abellion]

The guys at Symfony have a simple process that seems to work (never seen any BC break) :

• They never add new features in patch releases,
• They require with every PR some tests (if applicable) + a note in the CHANGELOG.md or UPGRADE.md,
• Before a minor release (every 6 months) there is a feature freeze something like 2 months before the release date and some RCs.

[m1guelpf]

I think what's wrong with our current release system is its versioning scheme, as every change will always have a breaking change for someone.

[thecodingdude]

@m1guelpf Honestly, the numbers laravel use are not fundamental to the current model of how Laravel is releasing changes. The current approach is to merge PR's, cut a release and then let the community use it, only reverting stuff that broke after the fact, ensuring there will be a follow up release with reverts.

It's generally unacceptable in software development to ship broken code - and you're right, any change can be considered 'breaking', even changing an integer from a 1 to a 0 or a return from true to false.

This is why there should be procedures in place to ensure, to the best of our ability, that never happens. Spending more time on PR's, ensuring they add tests, making an RC release and waiting a week or two all contribute to that process. It's not like Linus Torvalds ships his changes to the stable branch of linux every week, he actually takes time to let people test the latest changes using the RC process, after several RC's he then cuts the release to the mainline branch. If he did what Taylor does at the moment the kernel would be a complete disaster and simply would not function.

We can go around in circles about this all day, but judging by the reaction this PR has got so far people do care about this and all we want is for broken changes to never be shipped, it's not a difficult request and can be fixed in 20 seconds by simply not clicking "accept merge".

[mnabialek]

There is one more thing - it's not impossible to predict all the changes in real applications. Framework is just framework - it's impossible to predict all the aspects of its usage.

That's why if you have application you really care, you should have your own tests in application so in case there is a bug in framework or change of functionality you use, some of tests should fail and then you should either update your app code or stay on previous version of package.

Also keep in mind you have also always option to revert to previous release and in fact if your app is live you don't run composer update too much. Whenever you update packages you can expect problems so you should test your application - it's not just Laravel, it's every single package that is used in your application - well, most of packages have some bugs or unexpected behaviors and whenever you decide to update them to latest you can expect some problems.

[mfn]

Came about to contribute here, slept over night; have to realise almost everything has already been said :)

Upfront: I really do like this Project. I think Mr. Taylor built something amazing (technically as well as community, etc.) 👍

I think the following comments do stand out for me as the direction I'm thinking in:

• @sisve with Can releases please be properly tested?  #22833 (comment) and Can releases please be properly tested?  #22833 (comment)
• @Dylan-DPC with Can releases please be properly tested?  #22833 (comment)

My thoughts on this matter:

• The default should be master branch, as in: the development branch
• You don't PR/merge against any versioned branch and then merge up. You only always merge into master and pick for older releases
• Effectively any release is "resting" unless its fixing bugs
• Only LTS receive security bugs longer than non-LTS releases
• Zero open PRs for the sake of zero open PRs is bad for everything: the project, the quality, the contributions
• No code comes in without tests. Exceptions should be the exceptions of exceptions and be justified.
• Rejecting PRs for "no plans" is the worst response you can get.
  I tried to make useful and meaningful contributions, with good description what the intention/solved issue is. I only got "no plans" but no one can tell me what "the plan" is. This is absolutely discouraging and from reading closely the last month it's clear I'm not alone
  • Another disheartening example: the support of logging channels for 5.6. The original author put much energy, time and thought into it as well as the peers. Taylor, being dissatisfied with something (to my knowledge he didn't express exactly with what), comes along, throws it out of the window and does it all by himself. Maybe he is one of the few Rock-Star programmers on earth. Maybe the original author is fine with landing his vision of the logging into Laravel either way. Maybe this is really not how you work together in (any kind of) collaboration. Even in my company as a commercial entity I wouldn't do that to a co-workers PR even if I completely disagree with everything. The team gives appropriate feedback and the chance to do it right
• OTOH: the project receives a lot of low quality PRs:
  • No description or improper information
  • No tests
  • Bad architecture or not thought through. Sometimes just bad code. This takes a lot of wo-man-power to comb through the sand
    Laravel is a bit of disguise to newer developers I think. From the developer-users perspective, it's easy to get into it and to use. But the underlying architecture (at least in the parts I saw/know) is following more or less industries best practices, how the OO is designed etc.) and I get the impression many developers trying to contribute are not aware of this and easily make false assumptions and thus low-quality contributions
• There's also the issue of PRs changing…
  • code style (not just pure formatting, but think about the } else { issue
  • mass changes regarding how things should be done (e.g.: phpunit has many ways of doing one thing. Some people prefer this, some this way; case in point: how expected exceptions are tested for)
    What procedures/policies should be in place for such things?
• IMHO "internals" discussion doesn't really work. It's just a way to keep away people from laravel/framework issues but I hardly see any fruits really carrying over. And it complicates thing if you want to search for something, you may not realize there are related topics.
• I've nothing against/for a BDFL (or maybe I don't completely understand it) but why has this person be in 99% the only one merging stuff? I only saw Graham merging the smallest trivial stuff don't even remember others (Maybe Mr. Said?).
• Maybe the project needs more Lieutenants who are actively processing PRs besides BDFL
• Maybe Laravel has outgrown itself and needs re-positioning? Maybe I'm exaggeration 🤷‍♀️

[Dylan-DPC-zz]

IMHO "internals" discussion doesn't really work. It's just a way to keep away people from laravel/framework issues but I hardly see any fruits really carrying over. And it complicates thing if you want to search for something, you may not realize there are related topics.

It will work if "people with power" devote some time to it.

[deleugpn]

The default should be master branch, as in: the development branch
You don't PR/merge against any versioned branch and then merge up. You only always merge into master and pick for older releases
Effectively any release is "resting" unless its fixing bugs

BDFL. It's part of Taylor's process and I don't think any discussion here about it makes sense unless he is interested in changing his process. After much consideration on @thecodingdude reply to my comment, I can see that what people want here doesn't necessarily need to affect Taylor's process. Google Chrome / Chromium Project doesn't do bug-fixes / downward merge either. They only merge up and release a new version with fixes / features. Google Chrome version released October (3 months ago) is already out of support. However, they do have a canary, dev, beta and stable release process. That's why I said that even if Taylor wants to keep things as is, it's still possible by only working on the release process itself.

Zero open PRs for the sake of zero open PRs is bad for everything: the project, the quality, the contributions

When managing so many repositories (Laravel Framework, Laravel App, Dusk, Socialite, Passport, etc), having PRs hanging around would be a nightmare. Which brings back my original point: do we want to discuss whether Taylor should change his process or can we just focus on discussing the release process (which doesn't force him to change his process if he doesn't want to)?

[Dylan-DPC-zz]

When managing so many repositories (Laravel Framework, Laravel App, Dusk, Socialite, Passport, etc), having PRs hanging around would be a nightmare.

Why would it be a nightmare? Taylor merged Leo's PR after 4-5 months, so what's wrong in doing that for others as well?

[taylorotwell]

From now on I'll just be closing all PRs without good tests and explanation.

[jordyvandomselaar]

Not saying it's a bad thing, but having many open pr's open is what happens when you manage many different repositories.

[taylorotwell]

I will not leave open many PRs at one time. I just don't work that way and won't work that way. Period.

I have created a new saved reply I will be issuing to ALL PRs that are not VERY sufficiently tested and explained. This will save me some time in closing them and I will personally vow to be a lot tougher on PRs from now on.