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: ensure bulk action is done before checking for result #602

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

jjaffeux
Copy link
Contributor

Sometimes the target topic can be out of the viewport and this was causing failing specs.

Sometimes the target topic can be out of the viewport and this was causing failing specs.
@jjaffeux
Copy link
Contributor Author

Please merge if you approve

@davidtaylorhq
Copy link
Member

Were you able to reproduce the flaky failure?

According to the capybara docs, the visible: parameter relates to element visibility (e.g. things like display: none). So theoretically I don't think it help with anything viewport-related. (unless we had viewport-based cloaking/virtual-lists... but our topic-list doesn't have that).

For viewport-related stuff, capybara has obscured: true|false... but that defaults to nil, so the viewport shouldn't affect anything. 🤔

@jjaffeux
Copy link
Contributor Author

jjaffeux commented Nov 11, 2024

Were you able to reproduce the flaky failure?

According to the capybara docs, the visible: parameter relates to element visibility (e.g. things like display: none). So theoretically I don't think it help with anything viewport-related. (unless we had viewport-based cloaking/virtual-lists... but our topic-list doesn't have that).

For viewport-related stuff, capybara has obscured: true|false... but that defaults to nil, so the viewport shouldn't affect anything. 🤔

I had a lot of different failures, but regardless what you say makes sense... Looking at the latest failure on CI:

image

Clearly it should be unassigned here, but it's not.

@jjaffeux
Copy link
Contributor Author

nah... try_until_success it not going to make any difference here

@jjaffeux
Copy link
Contributor Author

@davidtaylorhq ok I think I know what is going:

  • start bulk operation
  • visit("/latest")

When we visit latest sometimes it probably cancels the request given it happens so fast. If we ask to wait for modal to be closed before we visit latest, that should always be good.

This is why we could have failure on line 52 or 72 as these are the two lines after visit("/latest")

Copy link
Member

@davidtaylorhq davidtaylorhq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@davidtaylorhq davidtaylorhq changed the title FIX: allows for target topic to be out of viewport DEV: ensure bulk action is done before checking for result Nov 11, 2024
@jjaffeux jjaffeux merged commit 7dd33d2 into main Nov 11, 2024
6 checks passed
@jjaffeux jjaffeux deleted the flakey-spec-2 branch November 11, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants