Problem/Motivation

Currently, the posthog_cookies JS aggregation test fails. It should be fixed!

Steps to reproduce

Proposed resolution

Fix the posthog_cookies JS aggregation test!

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork posthog-3509331

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

grevil created an issue. See original summary.

grevil’s picture

grevil commented

Maybe you find a way to fix the test. I couldn't.

Btw, we actually set the settings via the config object here, so @anybody's assumption, that the config object exists in another Drupal session than the one we control via UI inside the tests is incorrect!

grevil opened merge request !30

lrwebks’s picture

lrwebks commented

The problem might be caused by the JS aggregation itself, since all JS is merged into a single file then, which can of course be the reason for the missing library files. However, @grevil told me that the code for these cookie tests was mostly taken from the “COOKiES” module, so we might need to have a look regarding the correctness there too?

lrwebks’s picture

lrwebks commented
Assigned: lrwebks » Unassigned
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: Unassigned » grevil

We should also consider the option that the test is correct and with aggregation enabled posthog isn't currently handled correctly, because we need to explicitly disable aggregation, if that's needed for the knockout!

Like we do for other libraries:
https://git.drupalcode.org/project/cookies/-/blob/2.x/modules/cookies_gt...

I don't have the time currently to dig deeper, but either the test has wrong assumptions or the posthog_cookies implementation is wrong.

To find that out, we should manually tests the expected behaviour first, with aggregation enabled and see if it's right or wrong.

Can you follow my thoughts @grevil? We had similar cases with other libraries before...

grevil’s picture

grevil commented
Assigned: grevil » Unassigned

We should also consider the option that the test is correct and with aggregation enabled posthog isn't currently handled correctly

On a real site, everything works as expected. Only the test fails.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Ok then the test is wrong and also @lrwebks's assumption is correct, regarding the (exprected) aggregation.

grevil’s picture

grevil commented
Status: Active » Needs work

Ok, the js preprocessing (aggregation behavior) was overridden in our local dev environments "settings.local.php", meaning I have never seen any js / css aggregation in Drupal ever.

I adjusted the tests slightly, so that they should run theoretically now, but they don't.

As @lrwebks correctly mentioned, we can't check for any files directly if they are aggregated. This Test works in other cookies submodules, because there we manually disable any js aggregation, so we can morph the "data-src" attribute, but we don't need to do that here (see @anybody's example code here).

I disabled the file check, so now a different error appears:

Drupal\Tests\posthog_cookies\FunctionalJavascript\PosthogCookiesFunctionalJavascriptTest::testPosthogCorrectlyKnockedWithJsAggregation
TypeError: Cannot read properties of undefined (reading 'init_config_json')

Sounds like a racing condition to me? Maybe also something different. But I just tested it with ACTUAL js aggregation in my test site and the module works fine with js_aggregation enabled. So its a test error.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Category: Bug report » Task
Priority: Normal » Minor

Yeah I agree this looks like a race-condition or external sources (Posthog) are not loaded in tests (e.g. for security reasons that would make some sense to me).

drupalSettings.posthog_js is undefined in posthog_cookies.js

Let's come back here later, if we have an idea for the root cause. Good to know it actually works in real-life.

grevil’s picture

grevil commented
Status: Needs work » Closed (duplicate)

Fixed through #3558457: Overhaul Cookies integration using new Posthog best practices .

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

grevil’s picture

grevil commented
Status: Closed (duplicate) » Fixed

Status: Fixed » Closed (fixed)

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