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

test(cypress): add a new tests to verify secrets are mounted properly #3612

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lorenzo-cavazzi
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi commented May 6, 2024

This PR adds a new acceptance test for the new User Secrets feature

/deploy

@RenkuBot
Copy link
Collaborator

RenkuBot commented May 6, 2024

You can access the deployment of this PR at https://ci-renku-3612.dev.renku.ch

@Panaetius Panaetius force-pushed the build/secrets-in-sessions branch 4 times, most recently from df36d29 to db3d3c2 Compare May 7, 2024 18:17
@lorenzo-cavazzi lorenzo-cavazzi force-pushed the build/secrets-acceptance-test branch from 577b3ed to cb5ef62 Compare May 8, 2024 15:18
@lorenzo-cavazzi lorenzo-cavazzi marked this pull request as ready for review May 8, 2024 15:49
@lorenzo-cavazzi lorenzo-cavazzi requested a review from a team as a code owner May 8, 2024 15:49
@leafty
Copy link
Member

leafty commented May 10, 2024

I'm not sure a feature dubbed as "alpha" should have an acceptance test 🤔

Base automatically changed from build/secrets-in-sessions to release-0.52.x May 10, 2024 09:04
@Panaetius Panaetius requested review from a team as code owners May 10, 2024 09:04
Copy link
Member

@lokijuhy lokijuhy left a comment

Choose a reason for hiding this comment

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

I don't know if this is an appropriate PR to leave Changeling comments, since I didn't get to comment on the initial PR ?

CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
@lorenzo-cavazzi
Copy link
Member Author

lorenzo-cavazzi commented May 13, 2024

I don't know if this is an appropriate PR to leave Changeling comments, since I didn't get to comment on the initial PR ?

Sorry @lokijuhy the Files changed tab was showing a bunch of changes totally unrelated to the PR (now fixed -- I rebased to the release branch).

Do you mind moving the release/feature change requests to the release PR?

@lorenzo-cavazzi
Copy link
Member Author

I'm not sure a feature dubbed as "alpha" should have an acceptance test 🤔

Why not? The feature is labeled as "alpha" because we want to be reasonably sure everything is working as expected before users rely on it.

With this acceptance test, we check that other changes to the system don't accidentally break the feature. It seems reasonable to add one now since there are newly introduced components that are not covered by any other test.

Panaetius
Panaetius previously approved these changes May 16, 2024
@leafty
Copy link
Member

leafty commented May 27, 2024

@lorenzo-cavazzi the test seems to have failed in the last run

@rokroskar
Copy link
Member

That test seems to be somewhat flaky recently

@leafty
Copy link
Member

leafty commented May 27, 2024

It's the new test from this PR! So it should be checked if this is because there is an issue with user secrets in this PR's deployment or if the test is not robust.

@lorenzo-cavazzi
Copy link
Member Author

Yes, we should not merge this "as is". Somehow, the test fails much more frequently than it should. From the videos, it seems that starting the session takes forever, but when doing it manually it's almost always quicker.
My plan is to:

  1. Further investigate the problem
  2. Modify the useSession test since it's currently the slowest and it can be split into multiple tests; ideally, some of the changes from the "Start with options" page can also be combined, no need to start too many separate sessions since most of the time goes there

@lorenzo-cavazzi lorenzo-cavazzi marked this pull request as draft May 27, 2024 12:29
@olevski
Copy link
Member

olevski commented May 27, 2024

@lorenzo-cavazzi please target release-0.53.x

@lorenzo-cavazzi lorenzo-cavazzi changed the base branch from release-0.52.x to release-0.53.x May 27, 2024 15:53
@lorenzo-cavazzi lorenzo-cavazzi changed the base branch from release-0.53.x to master May 29, 2024 13:04
@lorenzo-cavazzi lorenzo-cavazzi dismissed stale reviews from Panaetius and lokijuhy May 29, 2024 13:04

The base branch was changed.

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.

7 participants