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

TASK: Extract job ID from saucelabs #3698

Merged
merged 9 commits into from
Jan 30, 2024

Conversation

markusguenther
Copy link
Member

@markusguenther markusguenther commented Jan 25, 2024

Description

This pull request addresses issue #2762.

In our end-to-end tests, we currently leverage Sauce Labs for recording video. However, the video link is accessible only to team members, limiting visibility for non-team members. To enhance transparency and allow contributors to view the recording without the need for local setup, this pull request introduces a script or GitHub Action.

The script or action automates the process of extracting the Sauce Labs video link and posts it as a comment on the corresponding pull request. With this enhancement, non-team members can easily access and review the video recording, gaining valuable insights into the end-to-end test results.

Changes Made

  • Added a script to extract the Sauce Labs video link.
  • Integrated the script to automatically comment on the pull request with the video link via CircleCI
  • The comment will be updated when the pipeline rerun and generates new recordings

@markusguenther markusguenther changed the base branch from 9.0 to 7.3 January 25, 2024 10:15
@github-actions github-actions bot added 7.3 and removed 9.0 labels Jan 25, 2024
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch 2 times, most recently from f173fa5 to 0a2b039 Compare January 25, 2024 10:54
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from 0a2b039 to d6e4482 Compare January 25, 2024 11:44
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from 5808d78 to 1740397 Compare January 25, 2024 17:26
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from fc1e086 to 721e508 Compare January 25, 2024 18:42
@neos neos deleted a comment from neos-bot Jan 25, 2024
@neos neos deleted a comment from neos-bot Jan 25, 2024
@neos neos deleted a comment from neos-bot Jan 25, 2024
@neos neos deleted a comment from neos-bot Jan 25, 2024
@neos neos deleted a comment from neos-bot Jan 25, 2024
@neos neos deleted a comment from neos-bot Jan 25, 2024
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from 995660a to 17baf49 Compare January 25, 2024 19:32
@neos neos deleted a comment from neos-bot Jan 25, 2024
@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from 17baf49 to b11853c Compare January 25, 2024 20:19
@neos-bot
Copy link

neos-bot commented Jan 25, 2024

🎥 End-to-End Test Recordings

These videos demonstrate the end-to-end tests for the changes in this pull request.

@markusguenther markusguenther force-pushed the task/add-circle-ci-videos branch from b11853c to ff94c49 Compare January 25, 2024 20:45
@neos neos deleted a comment from neos-bot Jan 25, 2024
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

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

Thank you so much. Thanks that you went thought that shell hell for us. I would have probably taken 1 hour for every shell line 😂 How do you do this???

@markusguenther
Copy link
Member Author

Thank you so much. Thanks that you went thought that shell hell for us. I would have probably taken 1 hour for every shell line 😂 How do you do this???

As you see, the commits has been force pushed several times and I needed lots of commits :D
But even when it was not so funny at some point, I wanted to finish it as I spent already so much time ;)

@mhsdesign
Copy link
Member

This is just ci and stuff. We wont be needing it for development as we dont develop on 7.3 anymore. So id say 8.3 should be it ;)

@markusguenther
Copy link
Member Author

However, I would like to explain why I think this PR is still useful and important. As you know, we still need to provide security patches for this branch, even though we are not actively developing new features or fixing bugs. For that purpose, it is helpful to have a reliable and fast way to run the tests and check the results. It also does not hurt to have it and is easy to upmerge.

@mhsdesign
Copy link
Member

Okay then let’s do it ;) I will not oppose any further.

@markusguenther markusguenther merged commit 7031363 into neos:7.3 Jan 30, 2024
9 checks passed
@mhsdesign
Copy link
Member

Hu but this caused a sideeffect that made the tests actually harder to read. I triggered now 5 test runs until i found out that the logs are written to app/Data/Logs/AcceptanceTesting.log. That is clever but not visible to the human eye :D

Also it seems the links were dead for the last prs i tried this feature on, sorry to inform you about that :/

@mhsdesign
Copy link
Member

Yes not seeing the current progress it a bit of a bummer, i would vouch for putting it into the log BUT most importantly keeping the output as it happens on the cli.

Also it seems the feature doesnt work any longer since a month now, maybe its too complex to maintain for us?

Having the github bot create a comment and stuff is great, but if its more stable to post the link to the recordings at the end of the run in the console output i would really like that.

Especially as the comments got me hyped into thinking someone reviewed my pr, but it was just the bot :O

@mhsdesign
Copy link
Member

Could you please revert this, or am i allowed to do so? ^^ As it A doesnt work and B obscures the logs and makes things harder to understand. Sorry that your effort dint pay out. But it guess this is just a too big complexity with relatively little gain?
... a Neos Ui stage would be another thing ...

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

Successfully merging this pull request may close these issues.

3 participants