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

refactor cypress/helper/util.ts #4651

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

Yokozuna59
Copy link
Member

@Yokozuna59 Yokozuna59 commented Jul 20, 2023

📑 Summary

Brief description about the content of your PR.

📏 Design Decisions

  • reorder functions declarations
  • rename mermaidUrl into createMermaidUrl
  • remove renderGraph and use imgSnapshotTest instead
  • rename openURLAndVerifyRendering into openUrlAndVerifyRendering
  • rename Utf8ToB64 to convertUtf8ToBase64
  • partially add JSDoc
  • combine imgSnapshotTest options assigment
  • change parameter default value into optional field
  • remove cy.get(svg) and {}

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 📓 have added documentation (if appropriate)
  • 🔖 targeted develop branch

@Yokozuna59 Yokozuna59 changed the title refactor cypress util refactor cypress/helper/util.ts Jul 20, 2023
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.79%. Comparing base (be1270d) to head (27cc5d3).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4651   +/-   ##
========================================
  Coverage    44.79%   44.79%           
========================================
  Files           25       25           
  Lines         5353     5353           
  Branches        27       27           
========================================
  Hits          2398     2398           
  Misses        2954     2954           
  Partials         1        1           
Flag Coverage Δ
unit 44.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@Yokozuna59 Yokozuna59 force-pushed the refactor-cypress-util branch 3 times, most recently from 2ec255d to 324986e Compare July 20, 2023 15:16
@Yokozuna59
Copy link
Member Author

The only thing left is JSDoc, I'm not really sure what to type, we could discuss it here.

@Yokozuna59 Yokozuna59 force-pushed the refactor-cypress-util branch 3 times, most recently from 207e597 to 5620e83 Compare July 20, 2023 15:41
* reorder function declarations
* rename `mermaidUrl` into `createMermaidUrl`
* remove `renderGraph` and use `imgSnapshotTest` instead
* rename `openURLAndVerifyRendering` into `openUrlAndVerifyRendering`
* rename `Utf8ToB64` to `convertUtf8ToBase64`
* partially add JSDoc
* combine `imgSnapshotTest` options assigment
* change parameter default value into optional field
@sidharthv96
Copy link
Member

sidharthv96 commented Aug 9, 2023

@Yokozuna59 is this blocked by anything? Can we finish this off before new PRs? Otherwise it'll conflict and it'll be more work for you.

Comment on lines +61 to +63
* @param url -
* @param options -
* @param validation -
Copy link
Member

Choose a reason for hiding this comment

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

You can remove all the @params without any valid descriptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added them as a placeholder so we could discuss what should we add.

@sidharthv96
Copy link
Member

sidharthv96 commented Aug 9, 2023

The only thing left is JSDoc, I'm not really sure what to type, we could discuss it here.

You can remove all the JSDocs that didn't have any existing comments.

But there are E2E tests failing. Those needs to be fixed.

@Yokozuna59
Copy link
Member Author

@Yokozuna59 is this blocked by anything?

@sidharthv96 Because E2E tests are failing. Not Really sure why but the options assignment isn't working as expected, for example: https://github.com/mermaid-js/mermaid/actions/runs/5796633792/job/15710598026?pr=4651#step:5:7346

TypeError: Cannot read properties of undefined (reading 'fontFamily')


Can we finish this off before new PRs? Otherwise it'll conflict and it'll be more work for you.

I'll give it a try.

You can remove all the JSDocs that didn't have any existing comments.

#4651 (comment)

@Yokozuna59
Copy link
Member Author

Because E2E tests are failing. Not Really sure why but the options assignment isn't working as expected, for example: https://github.com/mermaid-js/mermaid/actions/runs/5796633792/job/15710598026?pr=4651#step:5:7346

It should be resolved with this c74e55f

@Yokozuna59 Yokozuna59 marked this pull request as ready for review August 9, 2023 09:45
@Yokozuna59 Yokozuna59 self-assigned this Aug 9, 2023
@sidharthv96
Copy link
Member

@Yokozuna59 any clues why the tests are still failing? This has been open for a while, would love to merge this.

Copy link

netlify bot commented Nov 9, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 27cc5d3
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65fe908a764fe8000873716c
😎 Deploy Preview https://deploy-preview-4651--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Yokozuna59 Yokozuna59 force-pushed the refactor-cypress-util branch 2 times, most recently from 6ff4489 to 10a96c3 Compare November 9, 2023 16:26
@sidharthv96
Copy link
Member

@Yokozuna59 can you resolve the conflict?

@sidharthv96
Copy link
Member

Few tests with significant changes.
We should see if these are valid or not.

Sequence-diagram-links-shouldn't-display-unused-participants diff
Sequence-diagram-font-settings-should-render-multi-line-messages-aligned-to-the-right-when-configured diff
Sequence-diagram-should-render-loops-with-a-slight-margin diff
Sequence-diagram-should-render-a-sequence-diagram-with-boxes diff

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

Successfully merging this pull request may close these issues.

2 participants