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

Setup vitest as a dev-dependency to the plexus package #2561

Closed
wants to merge 1 commit into from

Conversation

mahdikhashan
Copy link

@mahdikhashan mahdikhashan commented Jan 7, 2025

Which problem is this PR solving?

Part of #340

Description of the changes

setup vitest, add related dependencies like testing-library and vitest/web-worker

How was this change tested?

Checklist

@mahdikhashan
Copy link
Author

@yurishkuro ,

after going through the readme and getting to know more about the package, i would like to start by adding a few snapshots to test the Diagraph. i'll not test anything related to GraphViz, but only plexus components.

why snapshot testing:

  1. we want to make sure components and specifically their generated output are correct
  2. snapshots are fast to run
  3. we can have different configs for the components (as we require)

what comes to my mind right now,

  • render a Diagraph component with HTML generated output (more focus on nodes)
  • render a Diagraph component with SVG generated output (more focus on edges)
  • investigate supplemental layers

later, i'll look into testing interactivity added by React.

@mahdikhashan mahdikhashan changed the title add vitest as a dev-dependency to the project add vitest as a dev-dependency to the plexus package Jan 7, 2025
@mahdikhashan
Copy link
Author

mahdikhashan commented Jan 7, 2025

ok, i have pushed my first test with the proposed idea - snapshot, as html and comparing.

another idea came to my mind right now, since its snapshots for components, it would be eaiser to have images to compare - then vitest, as i know doesnt provide any mechanism for it, another option would be to use an e2e framework, like playwright, it can generate images of the components, then we can compare them later within as well - the con is that its slower, we need to setup a brower (headless), in ci also it can take more time.

i'll wait for your feedback to proceed further @yurishkuro

@mahdikhashan
Copy link
Author

mahdikhashan commented Jan 7, 2025

hey @hari45678 - thanks for input, i think the answer to your comment is partly addressed in my earlier comment.

  • i saw your comment then suddenly its no more here! feel free to comment if you have inputs.

@yurishkuro
Copy link
Member

  • we want to move away from snapshot testing. You can read on the internet why it's a bad thing and @testing-library/react has specific recommendations of what to do instead of snapshots. We have an open issue for that as well.
  • make sure to always sign commits, your PR is not going to pass CI otherwise
  • the description says Resolves #340, which will close the issue of this PR is merged. The issue is not resolved by introducing a single test. Use Part of #340 instead.

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.63%. Comparing base (c9ccd8b) to head (c4cd190).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2561   +/-   ##
=======================================
  Coverage   96.63%   96.63%           
=======================================
  Files         255      255           
  Lines        7728     7728           
  Branches     2015     2017    +2     
=======================================
  Hits         7468     7468           
  Misses        260      260           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mahdikhashan
Copy link
Author

mahdikhashan commented Jan 7, 2025

  • we want to move away from snapshot testing. You can read on the internet why it's a bad thing and @testing-library/react has specific recommendations of what to do instead of snapshots. We have an open issue for that as well.
  • make sure to always sign commits, your PR is not going to pass CI otherwise
  • the description says Resolves #340, which will close the issue of this PR is merged. The issue is not resolved by introducing a single test. Use Part of #340 instead.

it makes sense to move from snapshots since they can be long and hard to debug -

i'll investigate the bare @testing-library/react approach, like testing with props and states - anyway

or we will go pr by pr for each steps, setup and then tests? in this case, let me finalise this pr as only vite setup.

looking forward to your reply.

@mahdikhashan mahdikhashan changed the title add vitest as a dev-dependency to the plexus package Setup vitest as a dev-dependency to the plexus package Jan 7, 2025
Signed-off-by: mahdikhashan <[email protected]>
@mahdikhashan mahdikhashan marked this pull request as ready for review January 7, 2025 18:01
@mahdikhashan mahdikhashan requested a review from a team as a code owner January 7, 2025 18:01
@mahdikhashan mahdikhashan requested review from mahadzaryab1 and removed request for a team January 7, 2025 18:01
@yurishkuro
Copy link
Member

what would be the definition of done for #340

Open to suggestions. Right now jaeger-ui shows code coverage at 97%, which is fantastic level, with the caveat that it actually excludes all of plexus. So the DoD could be achieving a certain level of coverage in plexus such that the overall repo coverage is at respectable level. I don't know now realistic it is, what do you think?

@mahdikhashan
Copy link
Author

what would be the definition of done for #340

Open to suggestions. Right now jaeger-ui shows code coverage at 97%, which is fantastic level, with the caveat that it actually excludes all of plexus. So the DoD could be achieving a certain level of coverage in plexus such that the overall repo coverage is at respectable level. I don't know now realistic it is, what do you think?

alrighty then, question that comes is whether the focus on coverage is realistic enough - all in all, i'm open to explore more after this pr.

@mahdikhashan mahdikhashan closed this by deleting the head repository Jan 8, 2025
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.

2 participants