-
Notifications
You must be signed in to change notification settings - Fork 6
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
refactor(client): make all fixtures use default options #2864
Conversation
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 is a good change to unify the structure of the fixtures. 🎉 I have a few requests for field name changes to clarify the semantics of the field, and a suggestion for a refactoring.
interface GetStatuspageInfoArgs extends SimpleFixture { | ||
overrides?: { [key: string]: unknown }; | ||
} |
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 like how you have put effort into unifying the types around some standard patterns. I think this could be added to that catalog as well:
interface FixtureWithOverrides extends SimpleFixture {
overrides?: { [key: string]: unknown };
}
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.
Done in 5127348.
const { | ||
fixture = "projects/project.json", | ||
name = "getProject", | ||
path = "", |
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.
path = "", | |
projectPath = "", |
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.
Done in e1b88f9.
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.
Also, the dashboard.spec.ts
fails, even locally, so I suspect an error was introduced into that test that needs to be fixed.
…rom the main settings page
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.
One of the changes requested in an earlier review was. missed, but this otherwise looks good and is very close to approval.
const response = { body: {}, statusCode: 500 }; | ||
cy.intercept( | ||
"GET", | ||
"https://*.statuspage.io/api/v2/summary.jsonr", |
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 is a mistake introduced by my PR, but I will fix it in #2871
Co-authored-by: Chandrasekhar Ramakrishnan <[email protected]>
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!
Details: