Fix failing e2e test for embeds: convo description selector

[patcon]

Re-ticketed from https://github.com/compdemocracy/polis/pull/872#issuecomment-786933155

Need to investigate, as realizing this is different from iframe issue. Seems to be just a too-short timeout, but maybe something else to it, with the data-test-id=description selector we're using (it's always that selector that's been failing)

https://github.com/compdemocracy/polis/pull/872/checks?check_run_id=1991111795#step:10:405

  1) Embedded Conversations
       "before each" hook for "hides description when user-can-see-description (ucsd) is OFF":
     AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-test-id="description"]`, but never found it.

[micahstubbs]

Should we disable this flaky test until it is fixed? I just tried to run the tests locally and noticed that it is failing still.

Steps to reproduce, for me:

cd polis
docker-compose up --build --detach

cd e2e
npm install
npm test

[patcon]

Skipping it is def fair game if no time to troubleshoot, although embeds are something that might be worth the time to salvage the test for, as embeds breaking is non-obvious and a pain to test manually. (if not now, i guess not sure when else the fix will happen 🤷 )

Fwiw, from looking at the video artifact attached to the failing test run: it seems unrelated to any sort of timeout (as guessed before), as the test is trying to access an admin page and not having access in that moment. Needs some interactive debugging locally, I'd say -- likely something to do with how the test is logging and and out. A wait step somewhere in the test might help, though not figuring out the true root of the login/logout issue

Happy to chime in if there are any specific questions, but also you're welcome to skip it at your discretion! 🙂

[colinmegill]

Important note:

⏳ This message appears when we are loading (and in that case is inaccurate, and should be a spinner).

https://github.com/compdemocracy/polis/blob/dev/client-admin/src/components/conversation-admin/conversation-config.js#L50

and

https://github.com/compdemocracy/polis/blob/dev/client-admin/src/util/component-helpers.js

need to be updated to reflect whether or not the app has received the data and the user is not authed, or whether we are waiting.

[patcon]

Ah good call! It may be related, but I think the test waits for awhile here before concluding it's not going to find what it's looking for -- it doesn't just fail immediately when "loading" (aka "no access") component flashes for a moment. So I think this is a legit "this cookie doesn't have access to dashboard" moment. (i.e. an issue with login/logout in test)

EDIT: yay long weekend :)

[colinmegill]

Loading, logged in, parent state / child props

https://github.com/compdemocracy/polis/blob/dev/client-admin/src/app.js#L106

[micahstubbs]

Good to see some movement on this one ☺

I would suggest trying to fix forward in some reasonable amount of time, say one more day or one more week.

If we aren't able to fix forward in that amount of time then I think we should disable the test until we can fix it or fix the underlying Behavior.

While every bug fix is important, reliably green tests are more important than any individual bug fix.

[patcon]

Sounds good, but sorry, to clarify, I was offering context on how it could be fixed, but won't be spending time on it myself. So it sounds like best to just add skip and merge

[micahstubbs]

@patcon ack, thanks for clarifying.

@colinmegill I think we should disable this flaky test and add a TODO Issue #873 comment in the codebase where we disable it.

I think that eventually we should try to fix this test and include it in the test Suite that runs all the time in CI.

[colinmegill]

I'm on board

[ballPointPenguin]

This is complete