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

Simplified intigration test for K8s in cypress #1201

Closed
wants to merge 48 commits into from
Closed

Simplified intigration test for K8s in cypress #1201

wants to merge 48 commits into from

Conversation

Shubham-Patel07
Copy link
Contributor

@Shubham-Patel07 Shubham-Patel07 commented Jan 25, 2024

What kind of changes does this PR include?

  • Fixes or refactors
  • A new challenge
  • Additional documentation
  • Something else

Description

Analyzed the tests written in java and rewritten in simplified cypress script for the challanges 5,6,7 and 33
Closes #1114

Checklist:

  • All the contributions made are solely the work of me and my co-authors
  • I tested the changes in this PR (if applicable)
  • I added unit tests to ensure my change works (when change in Java or on front-end code)
  • I added UI tests to ensure my UI changes work (when change in the overall UI, not needed if just adding a challenge)
  • The PR passes pre-commit hooks and automated tests

Copy link
Collaborator

@commjoen commjoen left a comment

Choose a reason for hiding this comment

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

Hi @Shubham-Patel07 ,
Thank you for your PR! Can you please have a look and fix the following 2 items please?

  • Right now the tests will be triggered always, while I would like to only have these specifically as part of the k8s GitHub workflow test. This means that these should be setup as separate tests outside of the existing e2e folder with their own config and triggered in the workflow. Can you separate them out and provide a separate config and trigger it from the GitHub workflow please?

  • Next, they don’t check that the default values are not in use (eg we will not know if they challenges actually work or not). Can you update the tests please?

Feel free to get in touch on Slack to discuss the details.

@Shubham-Patel07
Copy link
Contributor Author

Sure I'll do the changes

source ~/.bashrc
npm install
sudo java --version
./mvn spring-boot:run
Copy link
Collaborator

@commjoen commjoen Feb 1, 2024

Choose a reason for hiding this comment

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

see the unchanged code above: we already have everything running on minikube. CAn you launch the cypress tests against what is launched above please?
That means there is no need to install java and such again: just npx should do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry I didn't had a look on it, I'll work on it

@@ -0,0 +1,25 @@
// ***********************************************
Copy link
Collaborator

@commjoen commjoen Feb 1, 2024

Choose a reason for hiding this comment

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

do we really need this file or can we throw it away?

@@ -70,3 +70,69 @@ tr.solved {
.table {
--bs-table-bg: rgba(0, 0, 0, 0) !important;
}

/*toggle switch styling*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are now combining 2 different things in a pr, your previous pr with the ui change and the the cypress tests, can we please separate them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm really sorry actually it was not intentional, I accidentally pushed that

Copy link
Collaborator

@commjoen commjoen Feb 3, 2024

Choose a reason for hiding this comment

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

Can you remove all of it from this pr or create a fresh one please?

@Shubham-Patel07 Shubham-Patel07 marked this pull request as draft February 3, 2024 07:11
@Shubham-Patel07 Shubham-Patel07 marked this pull request as ready for review February 4, 2024 19:06
cy.dataCy(ThemeSwitchPage.DARK_MODE_RADIO).click()
cy.get(ThemeSwitchPage.DARK_MODE).should('exist')
cy.visit('/')
cy.dataCy(ThemeSwitchPage.DARK_MODE_CHECKBOX).then(($checkbox) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please remove all theme related items in the PR please?

@Shubham-Patel07 Shubham-Patel07 closed this by deleting the head repository Feb 5, 2024
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.

simplify k8s integration test (cypress or others)
2 participants