Skip to content

Update location of Moodle app behat tests steps #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

NeillM
Copy link
Contributor

@NeillM NeillM commented Jun 6, 2023

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

Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the contribution!

While "grepping" for uses I've seen that tests/app-setup.sh also have a few uses of the old moodlemobileapp repo.

Just guessing if we should also change them to the new moodleappbehat... pinging @NoelDeMartin .

Ciao :-)

@NoelDeMartin
Copy link
Contributor

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
Copy link
Contributor Author

NeillM commented Jun 13, 2023

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

@NeillM NeillM force-pushed the update-app-docs branch from be3e729 to 55f738c Compare June 13, 2023 11:03
@NeillM
Copy link
Contributor Author

NeillM commented Jun 13, 2023

@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
Copy link
Member

stronk7 commented Jun 13, 2023

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
Copy link
Member

stronk7 commented Jun 13, 2023

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
Copy link
Contributor Author

NeillM commented Jun 13, 2023

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
Copy link
Member

stronk7 commented Jun 13, 2023

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
Copy link
Contributor

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:

  • Provide behat steps to test mobile app functionality. This is what non-core developers would use this plugin for.
  • Provide acceptance tests to test the mobile app itself. This should only be useful for core developers, to avoid regressions when modifying the app or the LMS.

(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 NeillM force-pushed the update-app-docs branch from 55f738c to 1e4a497 Compare June 14, 2023 06:59
@NeillM
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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

@NoelDeMartin
Copy link
Contributor

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
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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

@NeillM NeillM force-pushed the update-app-docs branch from 2640184 to aab0c55 Compare June 14, 2023 11:24
@NeillM
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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
Copy link
Contributor

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.

The tests now simply check that the app can be started for both the lastest and development versions of the app work properly.

The Ionic3 versions are now well out of support.

It is likely most users would only be using these two versions of the app.
@NeillM NeillM force-pushed the update-app-docs branch from aab0c55 to a55ad3a Compare June 14, 2023 12:40
@NeillM
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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
Copy link
Contributor Author

NeillM commented Jun 14, 2023

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

Copy link
Contributor

@NoelDeMartin NoelDeMartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I think it looks good now :). I don't see anything wrong, so the failures are probably random as @NeillM mentioned.

@NeillM NeillM requested a review from stronk7 June 15, 2023 08:08
@stronk7
Copy link
Member

stronk7 commented Jun 15, 2023

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
Copy link
Member

stronk7 commented Jun 15, 2023

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 stronk7 merged commit 3815d6f into moodlehq:master Jun 15, 2023
@stronk7
Copy link
Member

stronk7 commented Jun 15, 2023

Sold, thanks!

@NeillM
Copy link
Contributor Author

NeillM commented Jun 15, 2023

Thanks both.

@stronk7
Copy link
Member

stronk7 commented Jun 15, 2023

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
Copy link
Contributor Author

NeillM commented Jun 15, 2023

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
Copy link
Contributor Author

NeillM commented Jun 15, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

NeillM commented Jun 15, 2023

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
Copy link
Member

stronk7 commented Jun 15, 2023

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
Copy link
Contributor

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: moodlehq/moodleapp#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
Copy link
Member

stronk7 commented Jun 15, 2023

@stronk7
Copy link
Member

stronk7 commented Jun 15, 2023

^^^ 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
Copy link
Contributor Author

NeillM commented Jun 16, 2023

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

  • line 692 of /lib/behat/classes/behat_config_util.php: call to behat_config_util->get_mobile_version_tags()
  • line 1027 of /lib/behat/classes/behat_config_util.php: call to behat_config_util->get_behat_profile()
  • line 409 of /lib/behat/classes/behat_config_util.php: call to behat_config_util->merge_behat_profiles()
  • line 129 of /lib/behat/classes/behat_config_manager.php: call to behat_config_util->get_config_file_contents()
  • line 308 of /lib/behat/classes/util.php: call to behat_config_manager::update_config_file()
  • line 195 of /admin/tool/behat/cli/util_single_run.php: call to behat_util::start_test_mode()
    !!`

Or perhaps this one:

`> [email protected] 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! [email protected] build:node-gyp rebuild`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] 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_31_52701Z-debug.log`

@stronk7
Copy link
Member

stronk7 commented Jun 23, 2023

Ping?

@NoelDeMartin
Copy link
Contributor

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
Copy link
Contributor Author

NeillM commented Jun 27, 2023

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
Copy link
Contributor

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
Copy link
Member

stronk7 commented Jul 4, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants