Support and document usage with the Moodle App

[NoelDeMartin]

I've been trying to use this for a plugin with mobile support, and fortunately it seems like very few changes are necessary to make it work.

The most tricky is changing the default browser though. I guess it would be better to just use chrome as the default, but this could potentially break some existing tests so I just decided to keep the same behaviour by default.

Other than a few changes in the code itself, I've documented the configuration. I've managed to get this working in Github Actions, but I wasn't able to test the Travis config because I don't have any more credits in my account 😅 I think it should work though, because the changes are minimal and very similar to the ones done for Github Actions.

[jrchamp]

Just a small note (not a full review), there's two references in here to postgres 10 that should probably be updated to postgres 12 to match the recent version bump.

[NoelDeMartin]

Hey @jrchamp, thanks for bringing that up. I initially implemented this before postgres 12 was used, and I didn't notice that this had changed so I didn't update it when rebasing. It's fixed now.

[stronk7]

Hi @NoelDeMartin ,

I like this change. Apart from the two suggestions that I've added as comments in the review... surely the unique point that I'm missing here is to have some own CI verifying that the new app support is working as expected. Maybe we can do something like:

• Add some "easy" mobile test to the fake plugin that we use for self-testing (tests/Fixture/moodle-local_ci).
• Ensure that both our Travis and GHA tests run it once? I'd suggest, for now, to add them just to the "MOODLE_401_STABLE" branch (add the 2 extra env variables there) and try it.

I've not tried, but I think that it will work and, that way, we ensure that we are always at very least running the apps stuff once.

What do you think?

Ciao :-)

[NoelDeMartin]

Thanks @stronk7 for your review! I've applied your suggestions and tweaked a couple of things to add tests, so please take another look.

Something I've had to do is removing the tests with MOODLE_38_STABLE, because the older behat configuration in the LMS is not compatible with the current version of the app. But I think it should be ok to remove those, given that 3.8 is already deprecated.

Dismissing the previous review because I'm doing a new one now

[stronk7]

Hi @NoelDeMartin,

A) before everything else... I want to share that, right now, we are (in the master branch) preparing the next big release (4.0.0) of moodle-plugin-ci. And a lot of changes have been applied to it already, with that, surely, being the cause of the conflicts that this PR is showing right now.

Also, note that there is the 3.x branch for any stuff that we want to get "backported" there.

For more information, #151 is where all the v4 progress is being shared.

Said that... maybe it wouldn't be a bad idea to make this one of those v4 features, so we keep 3.x stable and we don't need to go up-porting big things to master.

What do you think?

B) Second... one thing that I'm not sure I like (from a developer perspective) is the need to add any new step to install the moodle-local_moodleappbehat plugin and to start the moodlehq/moodleappdocker container. So I was guessing if, maybe... it could have sense to create a new command, say, behatapp that is able to, behind the scenes... do those things (install the local plugin and start the docker container).

Note that we want to do something similar with the phpunit and behat commands (not done yet, see #176), so the databases (for example) are only installed if the corresponding command is used (so the proposed change here for that new behatapp command would match nicely that behaviour (only install requirements if the command is effectively used).

What do you think?

C) Apart from the above points (or maybe related, heh), I'm not sure why we need specialised "dist" files. If, after all, it's the command (behatapp) the one going to decide what's instantiated and executed... surely the very same "dist" files will server for all commands. Or, in any case, no matter if B) is implemented or no... still... I think that it's more clear if we just keep one dist file, with commented-out stuff and/or more detailed comments in the "Explained" docs.

D) Also, there are a few details... in the proposed patch that I'm not sure to fully understand, like a) why do we need to specify the plugin in the self-testing, b) the lack of travis self-testing... note that the PRs DO run the travis self-tests, so we can ensure the app is working there too, c) the removal of 38_STABLE from the self-tests (it continues being supported officially in v4, d) changing the default browser... and surely some other small detail.

In any case... the main two points here are, IMO, A) and B). Aka:

A) Which the target of this PR is going to be. v3 and v4 or only v4.
B) If we try to embed all the dependencies within a new behatapp command, instead of having to install/launch them manually in the CI scripts.

Once those 2 are agreed, I think we can start working on everything with clearer goals.

Ciao :-)

[NoelDeMartin]

Thanks for checking it out @stronk7,

A) I didn't know this was happening, but yes I agree with adding it only for 4.0.

B) I also agree the DX would be better like that, but I didn't want to do so many changes (although I think at this point I've already changed a lot xD). What I don't like is using a separate command. Maybe we could do something like adding an --app flag or something in the install command? And then just use the normal behat command to run the tests.

C) Ok, if we end up removing all those special steps I think it makes sense to keep only one dist file.

D.a) I needed to do that because after installing moodlehq/moodle-local_moodleappbehat, the commands didn't work without specifying the plugin. I'm not sure if that's the expected behaviour or I was doing something wrong. You can see the error I got in CI here: https://github.com/NoelDeMartin/moodle-plugin-ci/actions/runs/3638952429/jobs/6141768058

D.b) I mentioned it in the description, but I can't test with Travis because I don't have any credits in my account :/ I guess we can leave this for the end and when everything else is working, I can ask someone for help or use one of the paid accounts we have for the apps team.

D.c) I mentioned this in my last comment, but LMS 3.8 is not compatible with the current architecture to execute app tests. I thought it was fine to remove since 3.8 is not a supported version anymore, but if that's a problem I'll try to configure it so that the tests are executed skipping the app tests.

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (693a638) 85.07% compared to head (2130c06) 85.15%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/Command/BehatCommand.php	0.00%	4 Missing ⚠️
src/Installer/InstallerFactory.php	33.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #176      +/-   ##
============================================
+ Coverage     85.07%   85.15%   +0.07%     
- Complexity      702      717      +15     
============================================
  Files            74       75       +1     
  Lines          2184     2229      +45     
============================================
+ Hits           1858     1898      +40     
- Misses          326      331       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[NoelDeMartin]

@stronk I've updated the code with the latest version, so you should be able to take a look again.

Besides applying what we discussed last time, I've also simplified the setup. For the most part, it should work just by setting a new MOODLE_APP env variable which is opt-in. If that variable is missing, everything should work the same way as before the PR.

Other than that, I also dropped the tests with the 3.10 version because they were giving some failures that I couldn't sort out. They were not related to the app either, and given that 3.10 is not a supported version anymore, it should be fine to ignore.

Everything else should be explained in the documentation, let me know if there is something else I should change.

[stronk7]

Hi Noel,

thanks for the great stuff and the patience! I've added a bunch of review-comments here and there, let's go discussing them.

Tomorrow I'll be doing some local tests (phar integration tests...) and surely will add some comment with a few points with whatever I find.

I'm sure that in a couple of iterations we can have this ready to become onboard!

Ciao :-)

[stronk7]

Ok, apart from the silly question commented in this review... that also made me think how can we try an older/different version of the app (because current setting is just true/not set). Can we? Or maybe it doesn't make sense at all?

As said, apart from that, I think this is ready to do. As commented in the previous review... I'll look apart if I can tidy a little bit the tests to ensure that the, now gone branches (38, 310) continue getting a little bit of love until we make the next major bump (raising PHP requirements). But that attempt doesn't stop this to progress.

Great job!

[stronk7]

And, yay, it landed, well done!

As said, I'll try to "revive" the 38 and 310 self-tests in another issue but, other than that, this is done.

Ciao :-)

[NoelDeMartin]

Thanks for integrating it @stronk7 :D.

About your question regarding old versions of the app, I don't think it's worth the effort because the app's lifecycle is not as long-lived as the LMS. When a new version is released, the old one is deprecated instantly and if we have to fix any issues we do it for the next release. That's why it only makes sense to test against the stable and development versions of the app.

Even so, if someone has a custom app and hasn't updated it for a while, they should still be able to make use of this by changing the default variables as written in the documentation.

And if you were mentioning it to configure some CI tests for multiple versions of the app... I don't think it's a good idea to test against the development version, because it can often be broken for known issues (right now for example we're migrating to a new version of Ionic and it'll be broken for a while). So I think it only makes sense to test it against the stable version, which is how it's already configured by default.