Don't show spinner for a failed analysis#1809
Conversation
|
I briefly forgot tests exist, but I'll try to write some that cover this. |
|
Here's some tests. If you think the level of detail in the details is too high or low feel free to say. |
| expect( | ||
| container.getElementsByClassName( | ||
| "codicon codicon-loading codicon-modifier-spin", | ||
| ).length, | ||
| ).toEqual(1); |
There was a problem hiding this comment.
I think toBeInTheDocument is sufficient for this, I don't think we need to explicitly test that we only have 1 element.
Should we also make the Icon more accessible by adding some alt text so we can use screen.getByRole instead? I'm not sure that should be part of this PR though.
There was a problem hiding this comment.
Definitely can do. I still need to access the first element of the array, but that works.
I don't have strong preferences about the icon. It was a bit annoying having to use this method to find the element, and it could be a bit fragile if we change the classes on the spinner. But also adding user-visible text to support the tests sounds a bit off too, perhaps. I think I'll leave it be for this PR at least.
There was a problem hiding this comment.
Definitely, I don't think we'd need to add user-visible text, but more something along the lines of this where we're adding an aria-label so screen readers understand it. We could potentially even use the Codicon component, although I'm not sure why that hadn't been done originally here. I suspect it's because of the font-size: 1em, but we'd need to test that theory.
2 participants
This PR avoids showing a spinning spinner when the variant analysis has in fact failed and won't make any more progress.
We're using the
completedAtfield to tell if the analysis is finished or not instead of the variant analysis status. This causes problems when the variant analysis failed to start a workflow because there were no repos to query. In this case it didn't get as far as creating a workflow run, and so there is nocompletedAtvalue. Possibly we should be settingcompletedAtin the API in this case, but we also can handle this in the extension.Also, if we just show the non-spinner bit of this component then we start showing the "view logs" link in cases when we shouldn't be. For example when the variant analysis fails before it can start the actions workflow, e.g. if there are no repositories to query. So as part of this I'm also making the "view logs" link option and only populating it when there are logs to link to.
Checklist
ready-for-doc-reviewlabel there.