Pull Request Testing

[Arran]

Some pull requests are over a year old, there's 23 pending, and a lot of them have this tag: "pr:needs-testing" I would be happy to test these if there was a way to test them without having to go to all the effort of compiling my own build.

Correct me if I'm wrong but 'master' is 1.6 and therefore basically all pull requests could be merged to master right now (or make a branch where all pull requests are merged to) and then we'd be able to test it all with the 1.6 nightly and then the whole community would have the ability to test all pull requests, then we can confirm whether or not the pull requests work. 1.6 is a long way off so there'd be plenty of time for any bugs to get discovered and if we're given a few weeks notice before a 1.6 release we can do general testing to see if any random bugs have popped up.

A lot more pull requests might actually get made if they got merged, back when I made some source pathces, it was demotivating to see my patches get ignored, then I just lost interest. The people with pending pull requests from months ago are probably feeling the same thing.

To be honest, all the pull requests combined could actually be good enough for a new release especially when there's "Added dynamic ped ID allocating" and if there was dynamic object ID allocating I'd be begging for 1.6 to get released. All you guys gotta do is check their code to make sure it looks legit, then I can do all the ingame testing.

Edited February 25, 2018 by Arran

[botder]

You know that I did Pull Request testing recently and I am planing to do more? There were 30 open pull requests in this month. The problem with PRs was that nobody felt responsible for testing and merging or maybe had no time to do so.

https://web.archive.org/web/20180112232210/https://github.com/multitheftauto/mtasa-blue

[Arran]

2 hours ago, Necktrox said:

You know that I did Pull Request testing recently and I am planing to do more?

I've been checking MTA commits multiple times a day for about 8 years, you know I updated wiki and mantis regarding those merged pull requests?

If you're planning on merging more that's great, just tell me what I can do to help, such as if there was a way for me to test the pull requests.

[Jusonex]

Any help is appreciated. You can download the build artifacts from AppVeyor:

1. Go to the PR GitHub site and scroll down.
2. Click "Show all checks" and "Details" in the "continuous-integration/appveyor/pr" row.
3. Switch to the "Artifacts" tab
4. Download and extract "InstallFiles.zip"

Providing a build that contains all pull requests that have the "pr:needs-testing" label assigned is a pretty good idea actually. I'm definitely going to discuss that with the other devs.

[Arran]

35 minutes ago, Jusonex said:

 

1. Go to the PR GitHub site and scroll down.

You're talking about https://github.com/multitheftauto/mtasa-blue/pulls right?

If so then I guess that is for developers only because I don't see what you describe.

[Jusonex]

No, I meant the specific pull request e.g. https://github.com/multitheftauto/mtasa-blue/pull/186

[Arran]

5 hours ago, Jusonex said:

No, I meant the specific pull request e.g. https://github.com/multitheftauto/mtasa-blue/pull/186

Ahh, I've tried one and my client crashes when joining the local server, gonna try another, 79 MB from appveyor.com takes 5 minutes. (Was trying "Add setElementCollidableWithType #181")

Also I just realised there isn't actually a 1.6 section on nightly.multitheftauto.com so does that mean master is 1.5.5? As someone whose been testing next release version since 1.0 it doesn't seem right that there is no 1.6 because... well all the major additions that MTA has had in the past, needed it so we could test them and for when there's non backward compatible changes.

Second attempt I'm now getting "Multi Theft Auto has not been installed properly, please reinstall." (Was trying "Added dynamic ped ID allocating(based on lopezloo pull request) #151")

Edited February 26, 2018 by Arran

[qaisjp]

On 26/02/2018 at 22:18, Arran said:

Ahh, I've tried one and my client crashes when joining the local server, gonna try another, 79 MB from appveyor.com takes 5 minutes. (Was trying "Add setElementCollidableWithType #181")

Also I just realised there isn't actually a 1.6 section on nightly.multitheftauto.com so does that mean master is 1.5.5? As someone whose been testing next release version since 1.0 it doesn't seem right that there is no 1.6 because... well all the major additions that MTA has had in the past, needed it so we could test them and for when there's non backward compatible changes.

Correct. master is the latest current-version (nightly), @Arran .  When we have features that need to wait for 1.6, we make a PR and attach a special label, like this: https://github.com/multitheftauto/mtasa-blue/pull/70

 

On 26/02/2018 at 22:18, Arran said:

Second attempt I'm now getting "Multi Theft Auto has not been installed properly, please reinstall." (Was trying "Added dynamic ped ID allocating(based on lopezloo pull request) #151")

do you have mtasa installed on that machine (a regular installation of the latest stable)?

[Arran]

I can't remember so I tested again, I copied my MTA installation, put the build files to over write, had to give users edit access to folder, server and client worked fine. Used the custom animation build this time.

[qaisjp]

FYI @Arran you can also get builds from nightlytest.mtasa.com - it's kind of a test website so it might move or go down or whatever, but it's perfectly usable. It'd be great to have you helping you out (again)!

[Arran]

Currently to test a pull request:

1. 4 minutes to download (appveyor download today was only going 400 kb/s so takes almost 4 minutes to download)

2. 1 minute to copy all the files into the MTA installation.

3. 30 seconds to modify the meta.xml each time.

4. 1 minute to launch client and server and join the server.

So at the moment it takes over 6 minutes just to get the PR build launched to test.

Can't PR's all just get merged into a testing branch so can test all PR's in 1 go? This is especially important because if a PR is tested 1 by 1 we're unlikely to notice any rare bug in the PR that could take hours of playing random stuff to come up.

[Dutchman101]

On 06/12/2018 at 17:42, Arran said:

Can't PR's all just get merged into a testing branch so can test all PR's in 1 go?

There would be a small risk of new features that have never been working side-to-side, interacting with eachother and causing issues and with that not resulting in the same test results as they will when tested on master.. this can go both ways, either the presence of additional issues or missing out on issues that would appear on master, because the surfacing of issue is being (partially) suppressed by another change.

If you put some cherry-picking work into it, like ensuring multiple PR's of the same feature category (or similar terrains) won't be in the testing branch at the same time, the chance of mishaps reduces but I believe at the end of the day you can't really avoid verifying your tests against master.

Besides the effort listed above, you may underestimate that there's even more manual work to be done, like reviewing the purpose of all PR's as some of them are either just suggestions based on the contributor's personal opinion or ideas (up to the dev team to decide if they would integrate it if the code was alright) or of such a low quality that it wouldn't even pass a first code-review.

I am just saying, it's not as easy as throwing all open PR's on a heap and putting them all in a testing branch. The real question is if the selection (which consists of a bit of concept, and general code review to probe if it has a chance at all) and investigating which ones won't possibly interfere with eachother, is even more worth it than sticking to how we're currently doing it.

Not to mention that all contributors developed their work based on master, not accounting for the design of other features they lacked knownledge of or that may make their integration better if it were in place at the time they wrote it. Basic principles in software development are that you spend half of your time thinking on how to do something, and the other half to actually write the code, so while the sudden presence of other PR features doesn't neccesarily cause interference, throwing them together can still degrade the quality of MTA's codebase. One PR may introduce changes which the other PR may benefit from or which forces the developer to take on another approach based on structural changes, or if not forced, would prefer that approach over their previous one (when they would write that feature today) because the other PR created new possibilities. Even if that PR isn't for a feature of the same category.

 

Bottom line

Your idea could be rather a development strategy, for which the additional time and effort put into the above vetting activities will perhaps be worth it. It may take some work to get a build with all suitable PR's thrown together to run, but the dev team can consider doing that as a public beta (like MTA 1.6) in which testing of all PR's is crowdsourced, and thus speeded up beyond a few individuals like you willing to actively test PR's.

But of course if the others deem that a good idea, the team capacity to set apart suitable PR's and resolve conflicts and interactions before that build will run has to allow that. Again, it's not as easy as it seems.