updated tests and added coverage for edit feature

[Carla-Moz]

This PR resolves the following issues:

• Write tests for the edit mode functionality
• Make sure edit mode is working properly

[netlify]

✅ Deploy Preview for mozilla-perfcompare ready!

Name	Link
🔨 Latest commit	cdde0d1
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-perfcompare/deploys/65e6202072a31a00081fa263
😎 Deploy Preview	https://deploy-preview-608--mozilla-perfcompare.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (8cabf8c) to head (cdde0d1).

Additional details and impacted files

@@            Coverage Diff             @@
##             beta     #608      +/-   ##
==========================================
+ Coverage   93.67%   97.87%   +4.20%     
==========================================
  Files          64       64              
  Lines        1422     1410      -12     
  Branches      223      219       -4     
==========================================
+ Hits         1332     1380      +48     
+ Misses         88       28      -60     
  Partials        2        2              

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

[julienw]

Thanks for this work!

As far as I understand the PR contains 3 different topics:

• The edit mode tests
• the result of npm audit fix
• the change of style of ExpandableRow
• (and also some change I don't understand in CompareWithBase, so I'm not sure where this goes)

Therefore before moving further, it would be good to make this PR only about the edit mode tests, so that we're not confused about all the other changes. I haven't reviewed the other topics anyway, so please remove them away from this PR and create separate PR for these. I'll gladly review them separately.

Now let's go for the meat of this issue!
I think the tests work well and bring some more coverage, especially around clicking inside the dropdown, so that's great.
I left a few easy comments that go basically into 3 buckets:

• make the file easier to work with (about setting the fetch mocks especially, or not doing twice the same tests)
• have fetch mocks for everything (see warnings in the console)
• bring some more coverage to our code (uncheck the elements in the dropdown instead of using the cross)

Happy to discuss :)

[julienw]

That is not needed, a <form> already has the implicit role form

[Carla-Moz]

Yes, true. But the test kept outputting an error, saying it can't findByRole('form'). That's why I added it.

[julienw]

I see, indeed I found this old issue with some more explanations: testing-library/dom-testing-library#1021
In short a <form> is not exposed with the role form implicitely if it doesn't have an accessible name. But this seems to have changed since then, so I filed this new one: testing-library/dom-testing-library#1293

Hopefully this issue will be fixed in the future then. In the mean time I think your change is valid.

Now I wonder if we could add an aria-label instead of a role. Surely this makes some sense, especially when we'll have both "Compare with base" and "Compare over time" in the future. Labeling the form could help a screen reader user to know which form they're focusing in, and I also believe the screen readers have functionality to list all forms and focus them.

Therefore I would suggest to add a aria-label (maybe aria-label="Compare with base" is good enough) instead of a role. What do you think?

[julienw]

Thanks, looks good to me, good work!

I'd like just 2 small last changes before merging :-) Then with these changes you can merge directly without an extra review.

[julienw]

I see, indeed I found this old issue with some more explanations: testing-library/dom-testing-library#1021
In short a <form> is not exposed with the role form implicitely if it doesn't have an accessible name. But this seems to have changed since then, so I filed this new one: testing-library/dom-testing-library#1293

Hopefully this issue will be fixed in the future then. In the mean time I think your change is valid.

Now I wonder if we could add an aria-label instead of a role. Surely this makes some sense, especially when we'll have both "Compare with base" and "Compare over time" in the future. Labeling the form could help a screen reader user to know which form they're focusing in, and I also believe the screen readers have functionality to list all forms and focus them.

Therefore I would suggest to add a aria-label (maybe aria-label="Compare with base" is good enough) instead of a role. What do you think?

[julienw]

nitpick: you can chain with the previous definitions:

global.fetch.get(
  ...
)
.get(
 ...
)
.get( 
  ...
);

[Carla-Moz]

Therefore I would suggest to add a aria-label (maybe aria-label="Compare with base" is good enough) instead of a role. What do you think?

@julienw This seems like a good solution to me, esp since I'm working on the Compare over time component. It makes sense to differentiate between the two.