search

moodlehq / moodle-docker

A docker environment for moodle developers
GNU General Public License v3.0
373 stars 244 forks source link

Update location of Moodle app behat tests steps #263

Closed NeillM closed 1 year ago

NeillM commented 1 year ago

I wanted to update the docs for the current locations of the Moodle app behat steps.

NoelDeMartin commented 1 year ago

Thanks for the ping @stronk7, yes I see there are a couple more places where this should be updated. If you want, we can merge this one and I'll open a new PR with anything else that needs updating.

NeillM commented 1 year ago

No worries, I will go and look at fixing them up.

NeillM commented 1 year ago

@NoelDeMartin It looks like the tests are failing because the moodleappbehat repository does not have a branch for each app version in the same way the old one did.

i.e. no v3.9.0 branch

Should they be there?

stronk7 commented 1 year ago

Thanks for fixing the remaining case in README, @NeillM . I'll wait for @NoelDeMartin about, maybe, leaving the tests for separate issue. I really was expecting, pretty much like Neill I imagine, that the replacement was all we needed.

But it seems that it's not only that so, yes, please Noel, confirm if we should delegate this to another issue and keep using the old repo for CIs in the mean time here.

Ciao :-)

stronk7 commented 1 year ago

Well, in fact... the README continue being wrong because there, the --branch "v$appversion" is used, and that's pretty much what the tests try to do and fail because the new repo does not have those versioned branches... so I'm not sure if this is acceptable change without fixing also that problem with the branches.

NeillM commented 1 year ago

I don't see --branch "v$appversion" in the README (or is the lack of reference there the problem?)

If the new way of doing things does not require the separate branches I will make sure that the main branch is always the one pulled done in the tests.

stronk7 commented 1 year ago

I don't see --branch "v$appversion" in the README (or is the lack of reference there the problem?)

Sorry my fault, I was reading that in the tests/app-setup.sh script and incorrectly assumed that it was part of the README / moodle-docker own scripts!

So just let's see what Noel opines about leaving the tests unmodified here (and delegate them to another issue), or if different branches (main, ci, latest...) are needed and that needs to also be documented here.

Ciao :-)

PS. Again, sorry for the confusion!

NoelDeMartin commented 1 year ago

Sorry for all the confusion about this 🙈️. I'll explain how we got here and the current status of the plugins, hopefully to make things clear. If you just care about what to do in this PR, go to the TLDR at the end.

This new plugin local_moodleappbehat is accomplishing 2 goals, which I realize is confusing but I'm not sure how to improve without making it even more confusing (like making yet another plugin).

The 2 goals are:

(The old plugin local_moodlemobileapp was also used to manage language strings, but now that's the only thing the old plugin does).

Before the new plugin existed, both things were done in the repository of the plugin. But we decided to move these to the app repository, to improve maintainability in the long term. Because of that, the new plugin is now auto-generated from files in the app repository so it's not modified directly. If you want to modify anything in this plugin, you'd do it in the app repo.

For the first goal (providing behat steps), we have the main and latest branches. These are the branches you'd use as a plugin developer to test your plugins with mobile support. There aren't specific versions because in the app we don't maintain previous versions, just the current stable (latest) and the one in development (main).

At the same time, though, the app should work with all the supported versions of the LMS. But now instead of doing that with branches, we have everything in a single branch and discriminate versions using tags like @lms_upto3.11 or @lms_from4.0. Again, this was done to improve maintainability.

For the second goal (providing acceptance tests), we have the ci branch. And this is the branch that will be used by bots in the tracker to test against regressions for new issues (spoiler, sorry :P).

In case you're curious, these are generated with the build-behat-plugin.js script and you can use the --exclude-features flag to generate both things or only the steps. And that's also what can be used to watch changes with during development.

TLDR: Having said all that, I think we should just use latest and main versions of the plugin here, and instead of running the core tests from the app, we should probably create some new *.feature files in this repository. After all, we are only testing that the plugin works with moodle-docker, so there isn't any need to actually test the app.

Again, if this all still sounds too confusing, feel free to just update the documentation and I'll look into it after merging this PR.

NeillM commented 1 year ago

Thanks for the explanation Noel.

Based on my interpretation of what Noel said I have changed the app tests so that the app-development suite will use the main branch and the other ones will use the latest branch.

I am not sure that I agree that the feature file should be in this repository though, since the steps in the moodleappbehat plugin may change over time breaking it, is seems that it would be better for the feature to be in the moodleappbehat plugin itself.

I have not done it yet, but I do wonder if it is maybe worth changing the app tests to be for just the latest and develop versions of the docker app and the equivalent for the source code versions (since that way in theory the tests are always running against supported versions) which means that they are less likely to need updating in the future.

I'm saying this based on the develop versions being reasonably stable test wise like the Moodle master branch is.

NeillM commented 1 year ago

I also realised I had not updated a link to the App acceptance testing documentation.

NoelDeMartin commented 1 year ago

I am not sure that I agree that the feature file should be in this repository though, since the steps in the moodleappbehat plugin may change over time breaking it, is seems that it would be better for the feature to be in the moodleappbehat plugin itself.

Well, that's true, but the steps should be fairly stable because we try not to break them since rewriting those would require a lot more work. For example, whilst all of this has moved from one plugin to another, the steps have remained the same. My idea with adding a .feature file in this repo was to just test the basic steps (such as I entered the app and not much else).

By the way, tests are passing now but that's because no tests are being run (given that none are included in the latest or main branches).

I have not done it yet, but I do wonder if it is maybe worth changing the app tests to be for just the latest and develop versions of the docker app and the equivalent for the source code versions (since that way in theory the tests are always running against supported versions) which means that they are less likely to need updating in the future.

Yeah I think that would make sense. The latest version of the app should be fairly stable, at least when it comes to testing basic things like entering the app. We are running behat tests in the CI pipeline in Github in all PRs, so they shouldn't break often.

Also, the ionic3 versions of the app have been unsupported for a long time, and even their equivalent in the LMS are about to reach end of life. So I'd also vouch for dropping that.

NeillM commented 1 year ago

That seems fair, I have added a feature file that just starts the app.

It is probably worth checking that you agree with how I have changed the app test grid.

I did not get rid of the runtime parameter as I imagine there will come a time when the live and development versions of the app will differ in that again.

I only test the app-development suite against the latest version of Moodle on the basis that it should not really be affected in any way that is different to the docker version.

Thinking about it more since I made my latest commits I wonder if the app tests could be further reduced by only testing against the lowest supported PHP versions for older versions of Moodle, while using lowest and highest PHP versions only for the current Moodle version (since I think the PHP versions are probably not affecting the app setup too much)

NeillM commented 1 year ago

It does look like my latest changes broke something in the app tests.

NeillM commented 1 year ago

It looks like my failures came around because I did not realise grep errors if there was no match found. I think it should be fixed now.

NoelDeMartin commented 1 year ago

I did not get rid of the runtime parameter as I imagine there will come a time when the live and development versions of the app will differ in that again.

To be honest, ionic3 and ionic5 is a bit of a misnomer; I think we did it this way because it was easier to distinguish than saying version <3.9.4 and >=3.9.5 of the app (yes, we did this in a patch version :/ mostly because appstores didn't allow using 10 for a minor, so we couldn't publish 3.10.0 version of the app).

So it should work with the "ionic5" runtime once we upgrade beyond ionic5. But it's ok if you want to leave it as is right now, I'll clean it up when we actually do the upgrade.

NeillM commented 1 year ago

It looks like the latest branch of the app source code is failing during it's npm run with:

`<--- JS stacktrace --->

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory 1: 0xa3ad50 node::Abort() [npm ci] 2: 0x970199 node::FatalError(char const, char const) [npm ci] 3: 0xbba90e v8::Utils::ReportOOMFailure(v8::internal::Isolate, char const, bool) [npm ci] 4: 0xbbac87 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate, char const, bool) [npm ci] 5: 0xd76ea5 [npm ci] 6: 0xd77a2f [npm ci] 7: 0xd8586b v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [npm ci] 8: 0xd8942c v8::internal::Heap::AllocateRawWithRetryOrFailSlowPath(int, v8::internal::AllocationType, v8::internal::AllocationOrigin, v8::internal::AllocationAlignment) [npm ci] 9: 0xd57b0b v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationType, v8::internal::AllocationOrigin) [npm ci] 10: 0x10a015f v8::internal::Runtime_AllocateInYoungGeneration(int, unsigned long, v8::internal::Isolate) [npm ci] 11: 0x1449379 [npm ci]`

The one using the main branch is passing though.

NeillM commented 1 year ago

It looks like it might be a random fail, since the same tests have passed in github actions for my repository

stronk7 commented 1 year ago

Great work, @NeillM and @NoelDeMartin !

I've relaunched the 2 failed jobs @ GHA, let's see if they end green.

I'll be performing the final review to this along the day. My understanding is that both you are happy with the current proposal, correct? (+1/-1 this comment, please).

Ciao :-)

stronk7 commented 1 year ago

I've relaunched the 2 failed jobs @ GHA, let's see if they end green.

Uhm... needed a couple of reruns to get them passing, hope it's some interim problem out there... anyway we are green now. As said... I'll look to this soon, ttyl!

Ciao :-)

stronk7 commented 1 year ago

Sold, thanks!

NeillM commented 1 year ago

Thanks both.

stronk7 commented 1 year ago

BTW,

I've just seen both the moodlehq and my clone GHA tests failing, like it was happening here. And both with 402_STABLE. I'm not going to re-run them, so we have the links:

They seem to happen more or less consistently in the tests/app-setup.sh with some problems running old npm becoming out of memory or similar... strange!

Ciao :-)

NeillM commented 1 year ago

I'm hoping that Noel might have some ideas why it is failing.

It is odd that it seems to be failing consistently on the latest branch of the app code (my understanding is that is the live version of the app) and not on the main one (and this is the in development version of the app).

NeillM commented 1 year ago

I imagine as a real backstop I could probably the highest version number that was being tested before my change instead of the latest branch

NoelDeMartin commented 1 year ago

We recently updated some dependencies in the main branch, so that may be one of the reasons for the difference. But other than that, I have no idea why it's failing. The only idea that comes to mind is that we're using node:14 here, whilst the tests in the app repo use .nvmrc which at the moment is exactly v14.15.0.

Seeing that it has something to do with running out of memory, and the fact that it works sometimes, I'm not sure how we could fix it :(.

NeillM commented 1 year ago

It looks as though it was also working fine of the v4.0.0 branch before these changes, I guess that was a couple of versions behind latest.

stronk7 commented 1 year ago

The node:14 image is, right now 14.21.3... one thing that surprises me is that the script forces npm@7 to be installed (and used, I imagine, have been checked that) when the default npm version for node 14 is npm@6.

Is there any reason for that?

Are we sure that the globally installed npm (v7) is getting precedence over the v6 coming in the image? Maybe it's needed to use 'npx npm' (note I don't know!).

In fact the workflow execution clearly says that the lockfile is old, created with old version of npm.... then... why are we forcing the use of newer npm (v7) ?

Just sharing thoughts, heh... do you also install npm v7 elsewhere, or just use the "bundled" (together with node) one? It's an strange upgrade, I would say (sure that for a reason, but... better ask).

Ciao :-)

NoelDeMartin commented 1 year ago

You're right, I totally forgot about that! We were using npm 7 by mistake in the past, but we updated the app to work with npm 6 in this PR: https://github.com/moodlehq/moodleapp/pull/3301 (about a year ago). So that can probably be removed, not sure if it will fix the flaky out-of memory thing though.

stronk7 commented 1 year ago

Let's see: https://github.com/stronk7/moodle-docker/actions/runs/5280735674 🤞

stronk7 commented 1 year ago

^^^ Heh, it seems that it has fixed the memory problem, now the FOUR "app-development" jobs are failing with different error (that I'm far away from understanding).

NeillM commented 1 year ago

I'm guessing that is this error

`Creating Behat configuration ...!!! Coding error detected, it must be fixed by a programmer: Unable to load app version from http://moodleapp:8100/assets/env.json !!! !! Error code: codingerror !! !! Stack trace: * line 768 of /lib/behat/classes/behat_config_util.php: coding_exception thrown

Or perhaps this one:

`> keytar@7.2.0 build /app/node_modules/keytar

node-gyp rebuild

Package libsecret-1 was not found in the pkg-config search path. Perhaps you should add the directory containing libsecret-1.pc' to the PKG_CONFIG_PATH environment variable No package 'libsecret-1' found gyp: Call to 'pkg-config --libs-only-l libsecret-1' returned exit status 1 while in binding.gyp. while trying to load binding.gyp gyp ERR! configure error gyp ERR! stack Error:gypfailed with exit code: 1 gyp ERR! stack at ChildProcess.onCpExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:351:16) gyp ERR! stack at ChildProcess.emit (events.js:400:28) gyp ERR! stack at Process.ChildProcess._handle.onexit (internal/child_process.js:285:12) gyp ERR! System Linux 5.15.0-1039-azure gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild" gyp ERR! cwd /app/node_modules/keytar gyp ERR! node -v v14.21.3 gyp ERR! node-gyp -v v5.1.1 gyp ERR! not ok npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! keytar@7.2.0 build:node-gyp rebuild` npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the keytar@7.2.0 build script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above. npm WARN Local package.json exists, but node_modules missing, did you mean to install?

npm ERR! A complete log of this run can be found in: npm ERR! /root/.npm/_cacache/_logs/2023-06-15T15_3152701Z-debug.log`

stronk7 commented 1 year ago

Ping?

NoelDeMartin commented 1 year ago

Hey, sorry I saw the errors but I don't know why they are happening, so I don't have much to add :/.

The only thing I can say is that the error saying Unable to load app version means that the app isn't running. But you'd have to look at the logs of the app container in docker to see what's going on.

I'll see if I can reproduce this myself, but off the top of my head I don't know what's going wrong.

NeillM commented 1 year ago

On Thursday before heading for a long weekend I did have a go at at making an ugly workaround (i.e. running the highest version of the app tested (v4.0.0) by the old set of tests when the latest app branch is called) but that fails on app start up.

https://github.com/NeillM/moodle-docker/tree/workaround-failures

The updated master branch did not fail for me when I pushed that up to my repository (I don't think it has ever failed against my github actions for some reason)

NoelDeMartin commented 1 year ago

Hey @NeillM and @stronk7, I've looked into this and found the problem. I opened a new PR, so check it out for the details #267

stronk7 commented 1 year ago

Thanks @NoelDeMartin, looking to #267 now, see you there!