Skip to content
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

Behat optimisations, addresses #765. #767

Merged

Conversation

marxjohnson
Copy link
Contributor

@marxjohnson marxjohnson commented Nov 22, 2024

This does a bunch of optimisations for the longest-running behat features. See the commit messages for details of what I've done. Further optimisations are possible, but I don't have time to work on this any further at the moment. Hopefully the groundwork of adding the behat_theme_boost_union and behat_theme_boost_union_generator classes will make it easy for others to make further improvements.

In my first run of these changes the tests completed in 2.5 hours, vs the 3.5-4 hours I was getting previously.,

[x] link your issue in the PR title, using the keyword 'resolves #ISSUE-NUMBER', e.g. 'Feature: Provide the ultimate user experience, resolves #42'
[x provide any further information that is relevant for peer review and not yet mentioned in the linked issue as a comment in the PR
[x] make sure that the 'Allow edits by maintainers' checkbox is checked when creating the PR. Otherwise, the peer reviewer would not be able to push any review changes to the PR and the communication overhead increases
[x] submit your PR in draft status to run the automated checks and review the results
[x] in case any checks fail solve the mentioned errors by pushing the corrected code to your PR-branch
[x] if all checks pass (or if you are unable to resolve the failing steps without any help of the review team), mark the PR as 'ready for review'

@marxjohnson marxjohnson force-pushed the 765-behat-optimisations-main branch 3 times, most recently from 3f6aa88 to c421855 Compare November 25, 2024 16:32
@marxjohnson marxjohnson marked this pull request as ready for review November 26, 2024 08:59
@abias abias mentioned this pull request Dec 6, 2024
26 tasks
@abias
Copy link
Member

abias commented Dec 6, 2024

Hi @marxjohnson ,

Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

Documentation

  • [Y] Commit Message understandable and issue referenced (via resolves keyword)
  • [Y] Patch author correct
  • [Y] CHANGES.md appended
  • [-] README.md appended
  • [-] Credits appended
  • [Y] Appropriate Comment quantity
  • [Y] Successful Moodle PHPDoc check

version.php

  • [Y] Checked if $plugin->version increment necessary (and increment done if necessary)
  • [Y] $plugin->release untouched

lib.php

  • [-] Only necessary functions

Languages

  • [Y] Only english strings
  • [-] Necessary magic strings added (e.g. capabilities)

Automated tests

  • [-] New functionality covered
  • [Y] No failing steps or scenarios

Mustache templates

  • [-] Example context exists

CSS and styles.css

  • [-] Bootstrap styles used if possible

Duplicated code

  • [-] Duplicated code is marked properly

Github action

  • [Y] All green

Commit history and scope

  • [N] Already single commit
  • [Y] Focus on single purpose
  • [Y] No surplus files

Additional aspects for Boost Union

  • [-] No usage of $theme->settings
  • [-] Usage of isset() checks when processing plugin settings in renderer / output code
  • [-] Usage of admin_setting_configselect instead of admin_setting_configcheckbox
  • [-] Modified mustache templates properly marked and .upstream template in place

Review result

Many thanks for this PR, these improvements are highly appreciated!
Your changes appear very well crafted to me and I have just pushed a small "Review changes" commit with minor changes to your branch.

Thank you especially for re-using the existing but up to now unused smartmenu_item::get_display_options() function. I have now fixed this function to only serve the purpose which you have introduced.


I will wait until the tests are green again and will then merge the PR into main and will also have a look at the backports into the stable branches.

If I may ask you one more thing:
Could you please create a follow-up issue here on Github with the things which you would have loved to optimize as well when you ran out of time in the end? That way, the tasks would be documented for anyone to pick it up as soon as time and budget allows.

Many thanks again,
Alex

abias
abias previously approved these changes Dec 6, 2024
@abias abias force-pushed the 765-behat-optimisations-main branch from 2eaffe0 to 7daf1ea Compare December 6, 2024 19:27
marxjohnson and others added 5 commits December 7, 2024 08:42
…oodle-an-hochschulen#765)

This creates generators and named selectors for smart menus and smart
menu items, plus some navigation URLs for the smart menu settings forms.
…dle-an-hochschulen#765).

This performs several optimisations to make these tests a lot faster.

Creation of smart menus and menu item has been changed to use
generators, rather than filling the forms each time.

Manual navigation has been replaced with `I am on the 'identifier'
'type'
page` as much as possible.

The assertions being made have been optimised. Previously, every
assertion of what was in a menu was opening each menu location and
checking for each item. Given that there's no option to display
different items when a menu is in different location, a lot of this is
redunant, and actually opening each menu takes time.

I have optimised this so that one scenario does a check that the menu
opens and the items are actually visible. Then each scenario tests than
an expected item exists in each menu (without opening the menu) and
checks for subsequent items just check existance in a single menu. This
balances coverage of the different functionality with speed and
conciseness of tests.
…hochschulen#765)

Navigation steps have been optimised a bit, creation of blocks
now uses generators, and @javascript has been removed from tests
that do not require it.

Only tests using the off-canvas block region need Javascript since it
opens the overlay. Otherwise, we are just checking whether block regions
exist or not under various circumstances, which can be done much quicker
without Javascript.
…le-an-hochschulen#765)

As with other smart menu features, this uses generators, navigation
steps and named selectors to speed up the tests. There is more that can
be done to the feature, as time allows.
@abias abias force-pushed the 765-behat-optimisations-main branch from 7daf1ea to a244a77 Compare December 7, 2024 07:42
@abias abias merged commit 2cba6af into moodle-an-hochschulen:main Dec 7, 2024
5 of 6 checks passed
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.

2 participants