-
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 non-failure run statuses #25286
Conversation
…24848-non-failure-run-statuses
…24848-non-failure-run-statuses
Thanks for taking the time to open a PR!
|
<template> | ||
<div | ||
class="flex flex-col w-full px-24px justify-center items-center align-middle" | ||
:class="{'flex-grow': ['PASSED', 'OVERLIMIT'].includes(status)}" |
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.
This class doesn't seem to do anything in the tests, but it does in the actual debug tab view 🤷🏻♂️
const props = defineProps<{ | ||
status: CloudRunStatus | ||
canceledAt: string | null | ||
canceledByFullName?: string | null |
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.
If anyone has tips on how to deal with this, I'd be happy to refactor. I guess when something is nullable in GQL, it can be either TheType | undefined | null
? This leads to weird stuff with TypeScript where it's not enough to just make the prop optional, you also have to specify that it could be null
. It's ugly
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.
I spiked into fixing this but it's going to be a chunk of work. There's a bunch of options we can configure: https://the-guild.dev/graphql/codegen/plugins/typescript/typescript-operations
The ones we need to figure out and standardize:
maybeValue
(default isT | null
, maybe we wantundefined
?)- we should do
immutableTypes: false
. This will stop generatingreadonly
which is a bit annoying, even if it's more technically correct
An alternative in the short term might be type Maybe<T> = T | undefined | null
. 🤷♂️
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.
Some comments/questions.
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.
Lots of little comments/questions. Looks good for the most part, just small cleanup items
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.
Please see my comment on timezones, I think getting the correct is critical.
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.
My comments have been addressed
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.
LGTM, thanks for addressing my comments 👍
User facing changelog
n/a, merging to a feature branch
Additional details
For some run statuses on the Debug tab, we need to display specific information, sometimes in addition to displaying the failing tests for debugging. This PR adds UI for the following run states:
CANCELLED
PASSED
ERRORED
NOTESTS
TIMEDOUT
OVERLIMIT
Steps to test
Ideally, you should create a run on your local Cloud instance for each of the statuses listed above. I was only able to actually re-create a few of them end to end (I couldn't re-create
TIMEDOUT
orOVERLIMIT
). I would suggest checking out the tests that I added here and viewing the Percy snapshots that were generated from those tests and comparing those to the Figma.There's an issue with the tests for
PASSED
andOVERLIMIT
where in the test they appear towards the top of the viewport instead of further down where they should be. However, if you actually view these states on the Debug tab, they show up correctly. There should be aflex-grow
class on these that create the correct height for the elements but it's not working in the tests.How has the user experience changed?
See Percy diff
PR Tasks
cypress-documentation
?type definitions
?