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

drupal-clean, drupal-demo - Switch to civicrm_install_cv #735

Merged
merged 8 commits into from
Dec 10, 2022

Conversation

totten
Copy link
Member

@totten totten commented Nov 3, 2022

Resubmission of #673 / #661.

I've started a local run of civi-test-run all on a drupal-clean site to make sure it still runs the full suite.

@totten totten changed the title drupa-clean, drupal-demo - Switch to civicrm_install_cv drupal-clean, drupal-demo - Switch to civicrm_install_cv Nov 3, 2022
@totten totten force-pushed the drupal_clean_install_cv branch 2 times, most recently from 3d57422 to 388da02 Compare November 4, 2022 23:45
@totten totten force-pushed the drupal_clean_install_cv branch 2 times, most recently from fd66bea to 7698fd0 Compare December 3, 2022 00:25
@totten
Copy link
Member Author

totten commented Dec 3, 2022

On new local build with drupal-clean, I tried civi-test-run for phpunit-e2e, phpunit-core-exts, and mixin. Almost everything was passing... except for search_kit's headless suite.

@colemanw Any chance you could checkout this branch and try something like:

civibuild create tmp --type drupal-clean 
civi-test-run -b tmp -j /tmp/junit phpunit-searchkit

It should generate something like this: https://gist.github.com/totten/f2efd3f45dbf5cea55d10bc0f7fece3f

@colemanw
Copy link
Member

colemanw commented Dec 5, 2022

@totten how do I checkout a branch before installing a build?

@totten
Copy link
Member Author

totten commented Dec 5, 2022

There's a secondary issue which I don't fully understand yet.

Installing dmastertest_qmzfj schema

Fatal error: Uncaught Error: Class 'Civi\Api4\SearchSegment' not found in /Users/totten/bknix/build/dmaster/web/sites/all/modules/civicrm/ext/search_kit/Civi/Api4/Service/Spec/Provider/SearchSegmentExtraFieldProvider.php on line 52

Error: Class 'Civi\Api4\SearchSegment' not found in /Users/totten/bknix/build/dmaster/web/sites/all/modules/civicrm/ext/search_kit/Civi/Api4/Service/Spec/Provider/SearchSegmentExtraFieldProvider.php on line 52

Context:

  • It regards PHPUnit tests in extensions.

  • It appears in 5.51 but not 5.56 or master.

  • It appears when SK is enabled in the base SQL snapshot (ie when civibuild restore gives you a system with SK active). This PR makes it appear more often -- before, it only appeared with drupal-demo and wp-dem; with this, it will also appears in drupal-clean (which is used by most CI test-runs).

  • It manifests with the test-suites in mosaico, extendedreport, and in newly-generated extensions.

    cd civix
    export CIVIX_WORKSPACE=/path/to/civicrm/ext/civixtest
    ./scripts/make-snapshots.sh --src --test --keep --version HEAD entity3
    
  • Related: https://test.civicrm.org/job/CiviCRM-Civix-Matrix/ (note the 5.51 column - passing on old/non-cv flavor of drupal-clean which lacks SK; failing on the old drupal-demo/wp-demo flavors which include SK).

After running tests more times than I care to recall... it seems this issue has gone away circa 5.52+ -- so maybe it's not critical from core pov.

(The prior comment though -- regarding SK's test-suite -- is still problematic and would affect master.)

@totten
Copy link
Member Author

totten commented Dec 5, 2022

@colemanw how do I checkout a branch before installing a build?

Not certain the question - here a few answers:

  • For loading 735, that's just needs to be loaded onto the buildkit.git repo first.
  • If you want to create a civibuild that includes a draft PR, then:
    • The easier way IMHO:
      civibuild create tmp --type drupal-clean --patch GITHUB_PR_URL
      
    • The more manual+generic way is to split the create step into smaller download and install steps, eg
      civibuild download tmp --type drupal-clean
      # Do any code manipulation you want
      # Then continue with installation:
      civibuild install tmp

@colemanw
Copy link
Member

colemanw commented Dec 5, 2022

@totten it's possible you stopped seeing the error in later versions because of civicrm/civicrm-core#24977

But whether you're still seeing it in 5.56 or not, here is an extra safeguard which IMO should be merged in regardless:
civicrm/civicrm-core#25114 should prevent those errors popping up again.

@totten
Copy link
Member Author

totten commented Dec 5, 2022

OK, cool, applying 25114 on 5.51 does fix the SearchSegment errors (at least, for the civix make-snapshot use-case). I also tried applying 24977, but it didn't seem to make much difference -- probably because it's focused on AJAX+Smarty entry-points (which shouldn't be in the callpath for an empty/skeletal test). 🤷

I also did a new run of phpunit-searchkit just to make sure these patches didn't influence that. This turned out to be a rabbit hole that can only be endured by dedicated rabbits. Following my own suggested steps above, I could not initially reproduce. I banged my head against different revisions of master, different versions of PHP, and enough rebuilds to make your vision go blurry -- just couldn't get it to reproduce. But I had seen it several times before, and it made no sense for the error to go away. Why couldn't I see it anymore?

Well... Finally found a thread to pull. Those failures appear to be an interaction between the various test-suites. So it recurs if you use drupal_clean_install_cv and run all the core-ext tests; thereafter, it will recur again one more time. But if only run the SK suite in isolation, then it passes.

## Flow 1
civi-test-run -b tmp -j /tmp/junit-solo phpunit-searchkit  ## Pass
civi-test-run -b tmp -j /tmp/junit-solo phpunit-searchkit  ## Pass
civi-test-run -b tmp -j /tmp/junit-solo phpunit-searchkit  ## Pass

## Flow 2
civi-test-run -b tmp -j /tmp/junit phpunit-core-exts       ## 10 failures
civi-test-run -b tmp -j /tmp/junit-solo phpunit-searchkit  ## 10 failures
civi-test-run -b tmp -j /tmp/junit-solo phpunit-searchkit  ## Pass

@totten
Copy link
Member Author

totten commented Dec 6, 2022

OK, the interaction involves civiimport<=>search_kit. This reproduces the problem:

git checkout drupal_clean_install_cv

civibuild create tmp --type drupal-clean
cd build/tmp/web/sites/all/modules/civicrm/ext/

(civibuild restore && cd civiimport && phpunit8 --group headless )
(civibuild restore && cd search_kit && phpunit8 --group headless )
## ^ Produces 10 failures, like https://gist.github.com/totten/f2efd3f45dbf5cea55d10bc0f7fece3f

@totten
Copy link
Member Author

totten commented Dec 6, 2022

Throwing in a cv flush does fix it, as in:

(civibuild restore && cv flush && cd civiimport && phpunit8 --group headless )
(civibuild restore && cv flush && cd search_kit && phpunit8 --group headless )

Interestingly, deleting sites/default/files/civicrm/templates_c doesn't help. (The folder isn't even populated after the test.) So there must be some other data-structure that leaks.

@colemanw
Copy link
Member

colemanw commented Dec 6, 2022

@totten possibly the packaged searches that come with civiimport?

seamuslee001 and others added 7 commits December 9, 2022 14:22
Use-case:

* Install with Civi 5.55
* Switch code to Civi 5.51
* Run `civibuild reinstall`

Before this patch, the step fails because `composer` is still out of date.

After this patch, it updates (similar to the original drupal-demo scripts)
@totten totten force-pushed the drupal_clean_install_cv branch from 7698fd0 to 8f6b54c Compare December 9, 2022 22:55
Context: We recently made `search_kit` mandatory and wanted to update the `drupal-clean` profile
to use the standard installer (which will enable `search_kit` sooner).

However, `civi-test-run phpunit-core-exts` exhibits a weird failure in this
mode (see #735).  The failure can be reproduced most quickly by comparing:

  cd ext
  (civibuild restore && cd civiimport && phpunit8 --group headless )
  (civibuild restore && cd search_kit && phpunit8 --group headless )

But if you throw in an extra `cv flush` after resetting the DB, it works fine. This
is strange, and it doesn't seem to be as simple as `templates_c`-clash. Still
not sure what resource is conflicted.

Regardless, this script is only used in CI, so the extra second of runtime
won't be noticed.
@totten
Copy link
Member Author

totten commented Dec 10, 2022

OK, I've used the simple cv flush workaround for the phpunit-core-exts issue.

Zooming back out to PR overall... I haven't really tested this across a full matrix, but I'm disinclined to do so -- it took a ton of time to reproduce/isolate the phpunit-core-exts locally, and I don't want to spend similar time on every other aspect of the matrix. If there are other issues, then CI will show them.

I should note tangentially -- there are recent/pre-existing failures (api\v4\Action\AutocompleteTest and api\v4\Entity\ConformanceTest) -- while there may be some thematic relation (related to SK mandate/shuffle), that appears to be separate -- and the changes here have no effect on those.

But we should keep an eye open on all versions (5.51, 5.56, 5.57) to see if other results change.

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