-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: IATR-M1.0 Debug Spec component update #25263
feat: IATR-M1.0 Debug Spec component update #25263
Conversation
… test to handle multiple groups
…date Merging with latest feature branch
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Various comments, mainly
- we can be more type safe, we can pair if you want to go over any of my comments
- I think we should avoid
!
- better to have a null check and be defensive. More verbose, but unless we are 100000% sure the property cannot be null/undefined, it's not worth the risk in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still working through other parts of the code, but wanted to share these comments.
The DebugSpecList
component is missing some items needed to be passed down to the child components. The DebugSpecListSpec
fragment is missing the specDuration
field. In the specs
computed value, all the information needed to be mapped in the return value for the spec
portion are not there. I think you may be able to just spread the entire specItem.spec
now and then override or add any other fields needed. Still looking at what might work.
|
||
it('mounts correctly for all icons together and has correct URLs', () => { | ||
cy.mount(() => ( | ||
<div class="flex flex-grow space-x-4.5 pt-24px justify-center" data-cy='debug-artifacts-all'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed an opportunity here, we could grab the very similar looking code here and here to make a DebugArtifactLinkList
component or something. We seem to have the idea of this component 3 places if you include this test, and the presence in the test tells me we want to see the artifact links grouped on their own like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things
|
||
const debugArtifacts = computed(() => { | ||
return props.failedTests.reduce<{[groupID: string]: {icon: string, text: string, url: string | null | undefined }[] }>((acc, curr) => { | ||
//TODO Update logic to not rely on defaulting to empty strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO a style preference or is does it functionally make a difference to something? If you don't plan to make this change, we can remove the TODO and maybe leave a comment explaining. If it's an important change, we should make an issue and put the issue number in the TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amehta265 I just realized that this is rendering all three buttons every time, but it should only render if the corresponding boolean value in the result is true and the url is present. I filed a separate issue to resolve this: #25319
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to approve as there are only small comments & we are going into a feature branch, I'll leave it up to you about which you implement.
In a general sense I think we can do more to label different content clearly for accessibility purposes and also to make the tests a bit easier to read . But I want to maybe come at that in a separate pass looking at this page as a whole a little further down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks super!
User facing changelog
This PR aims at updating the Debug Spec component and the Debug Failed component. It accomplishes the following:
It refurbishes the header section of the component to include a text icon and adds a
specsMetadata
section which provides more information about the spec duration, number of groups, browsers, operating systems, and testing types for the tests within the given spec.In addition, the failed component section is now created to support multiple groupings for a given test and hover states for each test instance.
Additional details
The most important thing to understand is the data schema passed into debugSpec (the "parent" UI component for this PR). The debugSpec component receives the schema below from
debugMapping.ts
:The
spec
is the current current spec that will be rendered by the debug Spec component.The
tests
are a mapping of thumbprints to the test represented by the thumbprint: Athumbprint
is a unique hash associated with each test. If the same test is run for two differentgroups
(e.g. Staging and Production) both tests will share the samethumbprint
but will each have uniquegroupId
pointing to the specific group each test is associated with e.g.{'unique1': [{...Test, grpId: '123'}, {...Test, grpId: '456'}] }
. This is an example of two tests sharing the same thumbprint but each have unique instances as the test was run for different groups. On the other hand a test with a single group would look this:{'Unique1': [{Test}]}
The
groups
are a mapping ofgroupID
to their corresponding group e.g.('123': {Staging}, '456': {Production})
.Each test will have at least 1 group associated with it. In this case we render the single group UI. On the other hand if there is more than 1 group then we show the multiple groups UI.
StatsMetadata.vue
is a reusable component that helps render a list of items separated by "." It is used in the spec component header section as well as the grouped failed test section.LayeredBrowserIcons.vue
is a component used to render multiple browsers in the header section of the component.GroupedDebugFailedTest.vue
is a reusable component that is rendered every time there are multiple groups associated with the same test.DebugArtifacts.vue
is a the component in charge of showing the hovered state iconsFor a better understanding refer to the images below:
Steps to test
This issue has 7 component tests:
DebugArtifacts
,DebugContainer
,DebugFailedTest
,DebugSpec
,GroupedDebugFailedTest
,LayeredBrowserIcons
,StatsMetadata
. Checkout to this branch to run the tests from the packages/app/ folder. To run these tests:Navigate to Component Testing and run the above tests
How has the user experience changed?
I have labeled different sections of the picture with file names to provide better understanding of which component is responsible for what UI.
Single Groups UI:
Multiple Groups UI:
PR Tasks
cypress-documentation
?type definitions
?