Problem/Motivation

Tests run through CI frequently fail on FunctionalJavascript tests.

This may happen through either GitlabCI or DrupalCI. Rerunning the test usually results in success.

Example failure:

There were 2 errors:

1) Drupal\Tests\project_browser\FunctionalJavascript\ProjectBrowserInstallerUiTest::testCanBreakStageWithMissingProjectBrowserLock
Error: Call to a member function getText() on null

/var/www/html/modules/contrib/project_browser/tests/src/FunctionalJavascript/ProjectBrowserInstallerUiTest.php:167
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

2) Drupal\Tests\project_browser\FunctionalJavascript\ProjectBrowserInstallerUiTest::testCanBreakLock
Error: Call to a member function getText() on null

/var/www/html/modules/contrib/project_browser/tests/src/FunctionalJavascript/ProjectBrowserInstallerUiTest.php:200
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

--

There was 1 failure:

1) Drupal\Tests\project_browser\FunctionalJavascript\ProjectBrowserInstallerUiTest::testModuleAddAndInstall
Failed asserting that a NULL is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/modules/contrib/project_browser/tests/src/FunctionalJavascript/ProjectBrowserInstallerUiTest.php:72
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Steps to reproduce

Run tests in DrupalCI or GitlabCI. May fail.

Proposed resolution

The failure occurs in an assert immediately after $assert_session->waitForElementVisible. I'm not an expert on FunctionalJavascript tests, but I'm wondering if it's a timing issue, and the test is trying the assertion before the element is fully loaded. Could the more complex Svelte scripts be more taxing on the browser engine causing the wait to complete before the element is fully ready?

Not sure if there is any way to add an extra delay to give it a chance to catch up?

Issue fork project_browser-3420552

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

lostcarpark created an issue. See original summary.

lostcarpark opened merge request !432

lostcarpark’s picture

lostcarpark commented

This change seems to be working quite well in GitlabCI. Since introducing even a very small delay, I have not seen any failures of FunctionalJS tests in GitlabCI.

However, in DrupalCI, even waiting a half second after the waitFor() call, the tests almost always fail.

lostcarpark’s picture

lostcarpark commented

Because I haven't been able to get FunctionalJavascript tests working reliably in DrupalCI, I suggest turning them off. They are running in GitlabCI, so running them in both environments is redundant.

This change does the following:

  1. Replace WebDriverTestBase with ProjectBrowserWebDriverTestBase, a subclass which adds a 1ms delay after waitFor calls, and seems to prevent the FunctionalJavascript tests failing in GitlabCI.
  2. Disable FunctionalJavascript tests in DrupalCI by setting testgroups: '', effectively telling DrupalCI to run no FunctionalJS testgroups.

I'm not sure 1 is totally necessary, but FunctionalJS tests seemed to be failing quite a lot in both systems, and adding the delay seems to make them pass reliably in GitlabCI.

Also not sure if I've put the new classes in the correct place. I tried putting them in the FunctionalJavascript folder, but ProjectBrowserWebAssert can't be an abstract class.

lostcarpark’s picture

lostcarpark commented
Status: Active » Needs review
lostcarpark’s picture

lostcarpark commented
Assigned: Unassigned » lostcarpark
Status: Needs review » Needs work
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

Had a conversation with testing expert extraordinaire Matt Glaman on Mar 6. We discussed several things to try:

  1. Move some of the FunctionalJavascript tests from PHPUnit to Nightwatch. Matt had an issue and simply switching to using Nightwatch provided some stability. A good approach here might be to switch one test (i.e. testPagingOptions() - one that's proved flaky in the past) and push. Run the pipeline like 6 times and see if it doesn't flake on any of them. This would be a proof of concept to move more tests into Nightwatch.
  2. Can we refactor the tests to remove dependencies on "statefulness" - like do component-level testing? Note, this would likely still be handled in Nightwatch AND would require a Nightwatch add-on that core doesn't use, so would mean getting another 3rd-party dependency into core. Maybe this is a good thing, as it would allow for core-level component testing, which may be something we need, so may be worth the lift. Sally Young (@justafish) might be a wonderful person to hit up to see what component-testing libraries might make sense - that is, maybe we try to use Jest instead of Nightwatch for this, etc.
  3. This one might be a good Band-Aid until a better solution comes along, but if perhaps the flakiness is caused by resource constraints on the GitLab runner, possibly, is there a flag where we can actually turn OFF the parallelization of the FunctionalJavascript tests job in GitLab CI, and force the tests to run serially? That is how tests run locally, and they're not flaky locally. Right? Worth a shot.

lostcarpark changed the visibility of the branch 3420552-functionaljavascript-tests-can to hidden.

lostcarpark opened merge request !444

lostcarpark’s picture

lostcarpark commented
Assigned: lostcarpark » Unassigned
Status: Needs work » Needs review

I have disabled the parallel running in .gitlab-ci.yml. I have run 3 times in a row without any failures. Probably should try a few more to be sure, but that seems good enough to move to Needs Review.

Should we leave the parallel section commented for now, on the assumption that we intend to restore it later, or delete it?

lostcarpark changed the visibility of the branch 3420552-xpath-checks to hidden.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Needs work

Is it possible to leave the other three types running in parallel?

fjgarlin’s picture

fjgarlin commented
Related issues: +#2829040: [meta] Known intermittent, random, and environment-specific test failures

Core has a fair share of intermittent and random failures too, see #2829040: [meta] Known intermittent, random, and environment-specific test failures.

Turning the parallel off is a good measure, another one could be to tweak the resources given to the jobs (see how core tweaks the KUBERNETES_CPU_REQUEST per job). More demanding jobs might need a higher value than less demanding. The default for contrib is relatively low, because that's enough for most modules, but it's totally ok to tweak as needed.

Also, having said all the above, simplifying the current GitLab CI integration (as proposed in the MR https://git.drupalcode.org/project/project_browser/-/merge_requests/444/...) is not a bad idea. This module will eventually be in core, so the less customizations we have for GitLab CI, the easier to move to core.

lostcarpark’s picture

lostcarpark commented
Status: Needs work » Needs review

By default, tests are allocated 2 CPU cores. As the Project Browser tests are quite intensive, this is giving inadequate resources.

Increasing the value of KUBERNETES_CPU_REQUEST to 16 will hopefully give the tests sufficient resources.

However, while I had several successful test runs, I did have one failure of the FunctionalJavascript tests, even with this change.

I'm hopeful that this will improve reliability, but I think we also need to look at moving some of the tests to Nightwatch.

fjgarlin’s picture

fjgarlin commented
Status: Needs review » Reviewed & tested by the community

MR looks good to me. RTBC.

Yeah, we're bound to have the occasional random failure. Even core can't get rid of them #2829040: [meta] Known intermittent, random, and environment-specific test failures, and I can assure you that a lot of hours of investigation have gone into those issues too. Clicking the occasional "re-test" should be ok, and hopefully, this change will make it more reliable and stable.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

OK this is a good incremental improvement which unblocks some other folks & tickets, so we should do it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.