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

CI: Split Visual Regression tests #537

Merged
merged 2 commits into from
Jul 1, 2024
Merged

CI: Split Visual Regression tests #537

merged 2 commits into from
Jul 1, 2024

Conversation

zeshanziya
Copy link
Collaborator

No description provided.

Copy link
Member

@hussainweb hussainweb left a comment

Choose a reason for hiding this comment

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

This LGTM. But why do you think the deploy step is failing? Did you try rerunning it? Maybe it was a random failure.

Also, it seems that the tests pass without the Percy token. I thought it would definitely fail with an error. Is there some way to verify that it does not talk to Percy at all (maybe even a mention in the documentation)? Honestly, I am not comfortable that it is silently swallowing this error.

Lastly, we should make the tests do something useful. Earlier, Percy was useful but that won't happen anymore in a regular run. This is for a follow-up.

@hussainweb hussainweb marked this pull request as ready for review July 1, 2024 12:42
@hussainweb
Copy link
Member

My (unlikely) theory that this being a draft PR had something to do with failure did not hold water. That said, there is nothing else that explains why this is failing. If the CLI token was wrong, even the drupal_test would have failed. I am thinking this is an isolated case and we should just merge it anyway. What do you think?

@hussainweb
Copy link
Member

I'm going to merge and see what happens. There's no point deeply analyzing something that may be isolated.

@hussainweb hussainweb merged commit bcd0209 into main Jul 1, 2024
3 of 4 checks passed
@hussainweb hussainweb deleted the axp-168 branch July 1, 2024 15:53
@zeshanziya
Copy link
Collaborator Author

This LGTM. But why do you think the deploy step is failing? Did you try rerunning it? Maybe it was a random failure.

Also, it seems that the tests pass without the Percy token. I thought it would definitely fail with an error. Is there some way to verify that it does not talk to Percy at all (maybe even a mention in the documentation)? Honestly, I am not comfortable that it is silently swallowing this error.

Lastly, we should make the tests do something useful. Earlier, Percy was useful but that won't happen anymore in a regular run. This is for a follow-up.

@hussainweb it failed because all 3 dev environments were in use. and I have noticed once it fails for that reason, we can not deploy the same ref again. There was an issue removing the dev env. I have created an issue (axelerant/platformsh-action#31) and fix it ASAP.

One approach I am thinking to tackle this issue going further, before deployment, we can check the available env calling platformsh api, and if all are utilized, let's not deploy at all, so if an env becomes available, it can be deployed again.

I will check if there are any ways we can alert/fails the test if percy token is not available

@zeshanziya
Copy link
Collaborator Author

My (unlikely) theory that this being a draft PR had something to do with failure did not hold water. That said, there is nothing else that explains why this is failing. If the CLI token was wrong, even the drupal_test would have failed. I am thinking this is an isolated case and we should just merge it anyway. What do you think?

Yes, nothing related to draft PR. It failed because all dev environments were in use.

@hussainweb
Copy link
Member

Are you saying that because the first time it tried to deploy and it was full, it kept failing repeatedly even after we freed up the environments? That was not the behaviour before...

@zeshanziya
Copy link
Collaborator Author

Yes, you are right. If a deployment fails due to the non-availability of an environment, we can not deploy the same ref again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants