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

UI: Restore proxy tag tests #6360

Merged
merged 22 commits into from
Sep 26, 2019
Merged

UI: Restore proxy tag tests #6360

merged 22 commits into from
Sep 26, 2019

Conversation

backspace
Copy link
Contributor

These are surely still intermittently failing on CircleCI,
but I’ll use this PR to hopefully finally determine why and
fix it.

These are surely still intermittently failing on CircleCI,
but I’ll use this PR to hopefully finally determine why and
fix it.
Since adding Percy, the proxy tag tests have yet to fail 🤔
I got a failure after a few attempts without it, maybe I
just didn’t try enough with?
It appears the problem is that despite my attempts to force
the first task in the list to be the one with the tag so
it can be asserted against, it’s not always the first. So
this should make every task be of kind proxy, so asserting
against the first will work.
That seems to have fixed things… 🤞🏻
@backspace
Copy link
Contributor Author

backspace commented Sep 24, 2019

I think this is a once-and-for-all solution to this intermittent failure, finally. I had thought the way I wrote the setup to the allocation detail proxy tag test would guarantee that the first task in the list was the one with the proxy tag, so the assertion on Allocation.tasks[0] would work. However, I temporarily added Percy and found on one repetition that it was the second task that had the tag:

image

This explanation is backed up by the fact that this failure that led me to remove these tests yet again happened only in the allocation detail tests and not the test that looks for the proxy tag in a task’s header.

Since the number of tasks is dynamic, rather than try to change the factories to ensure there’s only one, this changes all the tasks to have the correct kind, so they’ll all have the tag, and asserting that the first one has the tag should always pass.

I’ve pushed many empty commits to repeat the tests with different random scenarios and it hasn’t failed since, so hopefully this means total success!

Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Nice sleuthing.

Aside from the 💯 empty commits that need to be cleaned up, this looks good.

I had a suggestion but feel free to ignore it.

const task = server.schema.tasks.findBy({ name: taskState.name });
task.update('kind', 'connect-proxy:task');
task.save();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm cool with this, but it seems like the thing that was biting you is that the tasks table is sorted alphabetically by task name. So if you wanted to stick with the original plan of only having one task be the proxy, you could mimic the sort by name behavior.

Your call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You’re right, I got lazy after the struggle to understand what was happening. I’ve changed it to sort by name, thanks!

@backspace backspace merged commit e3d8e73 into master Sep 26, 2019
@backspace backspace deleted the f-ui/proxy-tag-tests branch September 26, 2019 16:50
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants