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

dev/drupal#176 - Move cache/integrationtest only used for testing into require-dev #25054

Merged
merged 1 commit into from Feb 2, 2023

Conversation

demeritcowboy
Copy link
Contributor

Overview

This isn't compatible with symfony 6, and is unlikely to be since they themselves recommend to use symfony-cache.

Also it doesn't need to be on a live site anyway.

Before

Can't install with symfony 6.

After

?Don't know yet - see what happens here.

Technical Details

It looks like it's only used to support one test. So if this doesn't work an alternate is just fork it or bring into core, or rewrite the test. Or possibly upstream will take a PR but it's not actively maintained.

Comments

@civibot
Copy link

civibot bot commented Nov 25, 2022

(Standard links)

@civibot civibot bot added the master label Nov 25, 2022
@seamuslee001
Copy link
Contributor

This seems ok to me @totten ?

@demeritcowboy demeritcowboy changed the title [WIP] dev/drupal#176 - Move cache/integrationtest only used for testing into require-dev dev/drupal#176 - Move cache/integrationtest only used for testing into require-dev Nov 26, 2022
@demeritcowboy
Copy link
Contributor Author

Rebased to remove the goofy commit.

So it looks like there was some issue with the drupal 8 matrix tests and that's why this is in require currently, but I admit I don't understand what the problem was. When I run the E2E Cache tests with this with drupal 9, it seems fine. So maybe it has to do with the build environment here?

@demeritcowboy
Copy link
Contributor Author

#16522

@demeritcowboy
Copy link
Contributor Author

I might close this since all it does is postpone the problem and for example you still wouldn't be able to run core tests on drupal 10. Either upstream needs to support symfony 6, or the code should be moved into civi some way.

@colemanw
Copy link
Member

colemanw commented Dec 4, 2022

@demeritcowboy this is stale

@demeritcowboy
Copy link
Contributor Author

Thanks.
I spent some time looking upstream and

  • It looks like symfony uses this to run tests but
    • (a) I can't find where they are running those tests. They have some test results online but it doesn't run everything it looks like.
    • (b) I can't see how it would work with symfony 6, so maybe they are not actively running those tests.

So going to leave this here as a possible solution.

@demeritcowboy
Copy link
Contributor Author

jenkins retest this please

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@seamuslee001
Copy link
Contributor

ok we can always make further modifications but I think this is fine and Jenkins is ok with it lets get it merged

@seamuslee001 seamuslee001 merged commit a667319 into civicrm:master Feb 2, 2023
@demeritcowboy demeritcowboy deleted the cacheintegration branch February 2, 2023 21:15
@totten
Copy link
Member

totten commented Mar 6, 2023

@demeritcowboy @seamuslee001 Following this patch, we've got new failures in CiviCRM-D8-Matrix. Note that phpunit-e2e is passing for drupal9-clean (9.3.x+) -- in Civi 5.59. But for Civi 5.60 and master, they fail.

There isn't a clean way to load the require-dev dependencies, is there?

In the past, we had this rule to specifically add it on D8/D9 test-builds. Do we need to back to that?

@demeritcowboy
Copy link
Contributor Author

My latest thoughts are since it's unsupported to copy the code into civi.

@totten
Copy link
Member

totten commented Mar 6, 2023

OK, so say we move it into civicrm-packages. You mentioned before that Symfony Cache also uses the tests. I guess there's no real conflict there, right? (Because we would never try to run the test-suite from symfony/cache in the context of a full Civi build.)

@demeritcowboy
Copy link
Contributor Author

I would probably choose to put it under tests instead of packages. It's only used for one set of tests, and doesn't need to be in a live site.

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