search

silverstripe / silverstripe-session-manager

Allow users to manage and revoke access to multiple login sessions across devices.
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

MNT Comment out sporadically failing behat assertions #109

Closed emteknetnz closed 2 years ago

emteknetnz commented 2 years ago

Issue https://github.com/silverstripe/silverstripe-framework/issues/10294

GuySartorelli commented 2 years ago

Is there an intention to uncomment these when we move to github actions? If so, this should be mentioned in a relevant issue somewhere so it doesn't just get missed - if not, then we may as well just remove them outright instead of commenting them out.

GuySartorelli commented 2 years ago

Is there an intention to uncomment these when we move to github actions? If so, this should be mentioned in a relevant issue somewhere so it doesn't just get missed - if not, then we may as well just remove them outright instead of commenting them out.

dhensby commented 2 years ago

What's the cause of sporadic failures? It'd be better to solve the root cause than remove the test

emteknetnz commented 2 years ago

No intention to uncomment just yet. Not sure why they're failing, have investigated. Passes locally, sometimes passes on travis, failed on github actions ci. It's possibly just a timing issue, so it could be considered this was just a poorly written test. We're still testing the core functionality i.e. Then I should see a "Successfully removed session." success toast

GuySartorelli commented 2 years ago

It sounds like there are some transitions at play... might be worth chucking in a And I wait for 2 seconds in before those instead to give the transitions time to take effect.

emteknetnz commented 2 years ago

I think it's the opposite problem, the transition happens though behat is too slow to catch it

Failure screenshot from https://github.com/emteknetnz/silverstripe-session-manager/actions/runs/2243371757 shows the "Sucessfully removed session" toast when the Then I should see the text "Logging out..." in the ".login-session__logout" element assertion was run

image

GuySartorelli commented 2 years ago

Ahhh, I see. In that case I go back to my original comment - I think we should either remove the commented out code outright (feel free to amend the comment to something like:

We cannot reliably test the "Logging out..." text or css transition to hide the logged-out session because of behat timing issues

or else if you want to keep the commented-out code, that implies to me that we may want to fix the timing somehow someday so an issue should be created to track that.

emteknetnz commented 2 years ago

Have updated comment

GuySartorelli commented 2 years ago

Can you please also either remove the commented out code (the comment now addresses it so there's no need to keep the commented out steps that will never run) or create an issue as I mentioned? I'm guessing the former is preferred here since the timing issue seems like something we won't be able to resolve.

emteknetnz commented 2 years ago

Updated