Skip to content
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

Merged
merged 31 commits into from
Nov 2, 2023

Conversation

leafty
Copy link
Member

@leafty leafty commented Oct 25, 2023

Details:

  • Fixtures now have sensible defaults and callers can provide only the options they need.
  • Added types for most common types of fixtures

Copy link
Contributor

@ciyer ciyer left a 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.

Comment on lines 143 to 145
interface GetStatuspageInfoArgs extends SimpleFixture {
overrides?: { [key: string]: unknown };
}
Copy link
Contributor

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 };
}

Copy link
Member Author

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 = "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path = "",
projectPath = "",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e1b88f9.

Copy link
Contributor

@ciyer ciyer left a 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.

@leafty leafty marked this pull request as ready for review October 30, 2023 07:58
@leafty leafty requested a review from a team as a code owner October 30, 2023 07:58
@leafty leafty requested a review from ciyer October 30, 2023 07:58
Copy link
Contributor

@ciyer ciyer left a 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",
Copy link
Contributor

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

@leafty leafty requested a review from ciyer November 1, 2023 14:28
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@leafty leafty merged commit e93ecae into master Nov 2, 2023
@leafty leafty deleted the leafty/refactor-cypress-tests-2 branch November 2, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants