MM-52487: fix more playbooks tests

[lieut-data]

Summary

Fix various Playbooks E2E tests.

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-52487

Release Note

NONE

[lieut-data]

/e2e-test

[mattermost-build]

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

[lieut-data]

/e2e-test

[mattermost-build]

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

[affkjuhyt]

Check title PR

[lieut-data]

/e2e-test

[mattermost-build]

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

[lieut-data]

These checks were broken upstream, but redundant anyway.

[lieut-data]

Some of these tests assume CRT behaviour.

[lieut-data]

Not sure if there is a better way to avoid this?

[saturninoabril]

I'd suggest to bypass this globally during user creation at

mattermost/e2e-tests/cypress/tests/support/api/user.js

Lines 262 to 264 in 6c7cea9

	if (bypassTutorial) {
	cy.apiSaveTutorialStep(createdUser.id, '999');
	}

- cy.apiSaveTutorialStep(createdUser.id, '999');
+ cy.apiDisableTutorials(createdUser.id);

[lieut-data]

Trying to set the now deprecated experimental feature flag caused issues, so just rip it out.

[lieut-data]

The previous check was sensitive to the order of events, so now I'm just checking that the channel loads more directly.

[lieut-data]

/shrug

[lieut-data]

Not quite sure how this test ever worked!

[lieut-data]

Same experimental flag removal as above.

[lieut-data]

Avoid an error trying to dereference on first load.

[calebroseland]

Could you expand on this a bit? It doesn't seem like this would be be necessary with the upstream null checks in both PlaybookRunDetails and RHSRunParticipants.

Suggested change

	if (!playbookRun) {
	if (!playbookRun?.participant_ids) {

[lieut-data]

I didn't track down exactly why, but myUser kept changing causing an infinite loop. In my test, the assertion that getProfilesByIds doesn't return current user profile was false, so I just simplified.

[calebroseland]

Wouldn't this cause a refetch whenever the run object is changed? Boolean(playbookRun) could be used in the deps array to match the if logic and mitigate that issue. Is tracking role still needed?

Suggested change

	}, [dispatch, playbookRun, playbookRun.participant_ids, role]);
	}, [dispatch, playbookRun.participant_ids, role]);

Cont'd

Refetching is already an issue with participant_ids getting a new reference with each run change. Since we're here, could consider fixing via:

Suggested change

	}, [dispatch, playbookRun, playbookRun.participant_ids, role]);
	}, [dispatch, playbookRun.participant_ids.join(''), role]);

"Better" might be client-side diffing up at the reducer-level to keep refs to unchanged properties, and "best" would be a distilled websocket diff with only the change that fired the event—either likely too much for right now.

Cont'd cont'd

This is the direction I would lean—it would fix the potentially-stale state by selecting profiles from redux, and only fetch profiles as-necessary.

    //...
    const participantsProfiles = useSelector((state: GlobalState) => {
        const profiles = playbookRun.participant_ids.map((id) => getUser(state, id));
        return profiles.sort(sortByUsername);
    });
    //...
    useEffect(() => {
        dispatch(getMissingProfilesByIds(playbookRun.participant_ids));
    }, [dispatch, playbookRun.participant_ids?.join('')]);

[lieut-data]

Thanks for the deep dive @calebroseland and double checking my work here! Coming off PTO, I admit I don't have answers to some of what I was thinking, e.g. with the !playbookRun check. I've re-run tests without it and can't reproduce an issue.

I'm inclined to adopt all your proposes, except perhaps .join(''), as I'm not sure how to work around the linter warning of not explicitly depending on participant_ids.

I've pushed a proposed change -- let me know your thoughts?

[calebroseland]

@lieut-data the change that you made will effectively work because of the sometimes-fetching getMissingProfilesByIds instead of the always-fetching getProfilesByIds.

While redundant network requests are mitigated, the root problem is still there—it will execute the effect whenever anything on the run changes. The ?.join('') part was to base the effect dep off of the contents of the array, rather than the reference—but indeed not idiomatic.

[lieut-data]

This should bring us closer to 100% for playbooks, but there's potentially still some breakage. Hoping this moves the needle and I can revisit anything else when I'm back from PTO.

[calebroseland]

/e2e-test

[mattermost-build]

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

[calebroseland]

Wouldn't this cause a refetch whenever the run object is changed? Boolean(playbookRun) could be used in the deps array to match the if logic and mitigate that issue. Is tracking role still needed?

Suggested change

	}, [dispatch, playbookRun, playbookRun.participant_ids, role]);
	}, [dispatch, playbookRun.participant_ids, role]);

Cont'd

Refetching is already an issue with participant_ids getting a new reference with each run change. Since we're here, could consider fixing via:

Suggested change

	}, [dispatch, playbookRun, playbookRun.participant_ids, role]);
	}, [dispatch, playbookRun.participant_ids.join(''), role]);

"Better" might be client-side diffing up at the reducer-level to keep refs to unchanged properties, and "best" would be a distilled websocket diff with only the change that fired the event—either likely too much for right now.

Cont'd cont'd

This is the direction I would lean—it would fix the potentially-stale state by selecting profiles from redux, and only fetch profiles as-necessary.

    //...
    const participantsProfiles = useSelector((state: GlobalState) => {
        const profiles = playbookRun.participant_ids.map((id) => getUser(state, id));
        return profiles.sort(sortByUsername);
    });
    //...
    useEffect(() => {
        dispatch(getMissingProfilesByIds(playbookRun.participant_ids));
    }, [dispatch, playbookRun.participant_ids?.join('')]);

[calebroseland]

Could you expand on this a bit? It doesn't seem like this would be be necessary with the upstream null checks in both PlaybookRunDetails and RHSRunParticipants.

Suggested change

	if (!playbookRun) {
	if (!playbookRun?.participant_ids) {

[saturninoabril]

Thanks Jesse! Overall looks good to me except for one request.

[saturninoabril]

I'd suggest to bypass this globally during user creation at

mattermost/e2e-tests/cypress/tests/support/api/user.js

Lines 262 to 264 in 6c7cea9

	if (bypassTutorial) {
	cy.apiSaveTutorialStep(createdUser.id, '999');
	}

- cy.apiSaveTutorialStep(createdUser.id, '999');
+ cy.apiDisableTutorials(createdUser.id);

[lieut-data]

Thanks @saturninoabril & @calebroseland! Let me run a fresh E2E test run with your changes, and I'll re-invite you to review.

[lieut-data]

/e2e-test

[mattermost-build]

Successfully triggered E2E testing!
GitLab pipeline | Test dashboard

[saturninoabril]

@lieut-data You might have noticed that the E2E above timed out. I did retry the jobs and looked good now - https://automation-dashboard.vercel.app/cycle/341841. It's having a timing issue about the availability of test docker image. I've opened a discussion to collectively address the issue - https://community.mattermost.com/core/pl/xnyoucdr9jfntm73jw8ui9jb6r and filed Jira ticket for us to track - https://mattermost.atlassian.net/browse/CLD-5772

[lieut-data]

Thank you, @saturninoabril! I took a look at the two remaining issues:

• One looks like a Playbooks crash, but I can't find any console logs either on the dashboard or after tracing it to the specific run where it ran (https://git.internal.mattermost.com/qa/cypress-ui-automation/-/jobs/1564831/raw). I also can't reproduce locally.
• The other is a flaky test that I'm also having a hard time reproducing locally. It would be interesting to have a video snapshot to playback this particular failure.

I'm inclined to merge this and keep an eye on things -- let me know your thoughts!

[saturninoabril]

Thanks Jesse! Sounds good to me and will keep watching too.

• Created a ticket to address adding video artifacts in GHA - https://mattermost.atlassian.net/browse/CLD-5784
  • Note: GHA E2E migration in progress - first with smoke tests - CLD-5698 - Add e2e smoketests #23590