search

mermaid-js / mermaid

Generation of diagrams like flowcharts or sequence diagrams from text in a similar manner as markdown
https://mermaid.js.org
MIT License
70.81k stars 6.35k forks source link

combine `cypress` and `vitest` coverage reports #4494

Closed Yokozuna59 closed 1 year ago

Yokozuna59 commented 1 year ago

Currently, Mermaid uses only vitest to report coverage, and vitest doesn't run renderer, svgDraw, etc. files, which shows the wrong results (57%):

Coverage Status   The main purpose of coverage is to know what have been test and highlight possible edge cases, but with these misleading reports, it would be hard.   Suggest solutions:  

  1. Create a script that takes reports from vitest and cypress coverages and then combines them.   Here is an example for someone who combines jest and cypress coverage reports.  

  2. Make vitest handle both headless and unit testing; that will show better coverage, and from vitest.dev:  

    We suggest using Vitest for all headless logic in your application and Cypress for all browser-based logic.

    And:

    We believe that Cypress isn't a good option for unit testing headless code, but that using Cypress (for E2E and component testing) and Vitest (for unit tests) would cover your app's testing needs.

  3. Migrate to a new tool for both types of testing.  

  4. Migrate to the new visual testing tool for faster reports.

What do you think? Do you have something else in mind?   IMO, the first solution is good, but it will take more time to report, so the second looks like a better solution.

sidharthv96 commented 1 year ago

Create a script that takes reports from vitest and cypress coverages and then combines them.

Here is an example for someone who combines jest and cypress coverage reports.

That blogpost has everything we need to combine our coverage reports. So we should be doing that ASAP.

We suggest using Vitest for all headless logic in your application and Cypress for all browser-based logic.

This is what we're doing now. We cannot use vitest to test rendering as jsdom doesn't have the things we need. We need a browser to test rendering.

  1. Migrate to a new tool for both types of testing.

I'm not aware of any tools that does this, any recommendations?

  1. Migrate to the new visual testing tool for faster reports.

We have a LOT of image tests. Any migration would require

  1. Change of code. Doable, but a lot of grunt work. There's like 200+ places where cy. is used.
  2. Migration of approved data in Applitools. If we can ensure the rendering tests works, we could have an easier time here, but still a manual review would be required.
  3. Cypress has a hosted service (free), which allows us to verify tests easisly. The next best contestant in the space (playwright), lacks this. We'll have to download a trace file and run a command to view what happened.

I think we should really be doing #1 ASAP.

sidharthv96 commented 1 year ago

Coverage support added to E2E tests https://github.com/mermaid-js/mermaid/pull/4498 as our build process is not that straight-forward.

Accepting PRs for merging the coverage results.

sidharthv96 commented 1 year ago

The coverage actually revealed many old functions that could possibly be removed.

Yokozuna59 commented 1 year ago

That blogpost has everything we need to combine our coverage reports. So we should be doing that ASAP.

The blogpost only combined json coverages, doesn't show how to combine text, html, and lcov. Not sure about text or lcov, but I'm sure html is essential.

This is what we're doing now. We cannot use vitest to test rendering as jsdom doesn't have the things we need. We need a browser to test rendering.

Oh, I thought it was possible.

I'm not aware of any tools that does this, any recommendations?

Not yet; I haven't looked really well, tbh.

We have a LOT of image tests. Any migration would require

  1. Change of code. Doable, but a lot of grunt work. There's like 200+ places where cy. is used.

  2. Migration of approved data in Applitools. If we can ensure the rendering tests works, we could have an easier time here, but still a manual review would be required.

  3. Cypress has a hosted service (free), which allows us to verify tests easisly. The next best contestant in the space (playwright), lacks this. We'll have to download a trace file and run a command to view what happened.

I'm aware it would be a real headache, but cypress testing really takes much time to test, about ~10 min, and adding coverage reports well increase it more and more.

About the third point, how about vitest-preview? Not really sure if it was what you were talking about.

Coverage support added to E2E tests #4498 as our build process is not that straight-forward.

Accepting PRs for merging the coverage results.

Great! I was working on it locally but since you have an open PR, I'll wait to be merged then figure out a way to merge them.

But as I said before, just json coverage isn't enough, it might to need more work than expected.

The coverage actually revealed many old functions that could possibly be removed.

Let's leave that when standardizing definitions, since we're already converting and cleaning files.

Yokozuna59 commented 1 year ago

@sidharthv96 Where should we discuss diagram definitions? I guess discussing the structure in each diagram PR with no track if changes would be hard for reviewers/contributors to track and discuss the new structure, should we create an issue? And if the issue was clean enough we could refer it in Adding Diagram page, and create the cli script following the discussed structured, am I right?

I've almost getted all files need to be changed/modified when creating new diagram and writing description for each with need attributes/fields. Should I create the issue?

sidharthv96 commented 1 year ago

I think you can generate the other kinds of reports from the combined json.