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: [M3-7557] - remove console logs from e2e tests #10506

Merged
merged 3 commits into from
May 24, 2024

Conversation

AzureLatte
Copy link
Contributor

@AzureLatte AzureLatte commented May 22, 2024

Description 📝

Some of our Cypress tests have stray calls to console.log and similar functions (console.info, console.error, etc.), and these should be removed.

Changes 🔄

Remove console.log functions in the spec files (cypress/e2e/*/)

How to test 🧪

yarn cy:run -s "cypress/e2e/core/longview/longview.spec.ts,cypress/e2e/core/images/create-linode-from-image.spec.ts"

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@AzureLatte AzureLatte requested review from a team May 22, 2024 17:38
@AzureLatte AzureLatte self-assigned this May 22, 2024
@AzureLatte AzureLatte requested review from jdamore-linode, carrillo-erik and cliu-akamai and removed request for a team May 22, 2024 17:39
@AzureLatte AzureLatte requested review from a team as code owners May 22, 2024 17:40
@AzureLatte AzureLatte requested review from dwiley-akamai and hkhalil-akamai and removed request for a team May 22, 2024 17:40
Copy link

github-actions bot commented May 22, 2024

Coverage Report:
Base Coverage: 81.65%
Current Coverage: 81.65%

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

The diff LGTM and doing a search for console. in the codebase didn't show any other results in cypress/e2e/*/. The tests did fail for me locally though due a timeout so I'll approve once the remote tests pass

@@ -154,15 +127,6 @@ describe('longview', () => {
cy.findByText('Waiting for data...');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Install Longview on Linode by SSHing into machine and executing cURL command.
installLongview(linodeIp, linodePassword, installCommand);

In this case we still want to call installLongview, we just don't want to log its output. (This is going to be moot soon thanks to M3-8107, but I'm not sure this test will pass reliably in the meantime without this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I reverted the changes and put the installlongview() back.

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Thanks, @AzureLatte!

@AzureLatte AzureLatte merged commit 7b2220e into linode:develop May 24, 2024
18 checks passed
@AzureLatte AzureLatte deleted the M3-7557 branch May 24, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants