-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Restructure API namespace for Cy13 #5053
Conversation
* .readFile() is now a query * Add section on new .readFile() superpowers * Fix page slug for readFile * Prettier run
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Co-authored-by: Zach Bloomquist <git@chary.us>
docs/api/commands/exec.mdx
Outdated
- `cy.exec()` will only run assertions you have chained once, and will not | ||
[retry](/guides/core-concepts/retry-ability). | ||
- `cy.exec()` asserts that the command exits successfully. If the command exits | ||
with a non-zero exit code, `cy.exec()` will fail the test. |
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 this is the case, why does it yield stderr? Users can assert of use that value?
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.
Many commands write to stderr even when successful. curl
is one example.
https://askubuntu.com/questions/944788/how-does-curl-print-to-terminal-while-piping
When you are using curl to open an URL, you'll get two output:
The status of the curl itself. The contents of that URL.
Curl should use a way to show these two separately otherwise processing of the real output (URL's content) would be hard and I'll end up with unnecessary contents (curl's status).
So it uses stderr for its status and stdout for the content.
You wouldn't usually curl
with cy.exec
, but it's a common pattern in linux commands.
docs/api/commands/writefile.mdx
Outdated
@@ -213,8 +213,7 @@ cy.readFile(filename, null).then((obj) => { | |||
|
|||
### Assertions [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Assertions) | |||
|
|||
- `cy.writeFile()` will only run assertions you have chained once, and will not | |||
[retry](/guides/core-concepts/retry-ability). | |||
- `cy.writeFile()` has no implicit assertions. |
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.
above doesn't it say it verify the file contents were successfully written to disk?
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.
writeFile yields null
, and makes no implicit assertions about the subject it yields.
In my mind, and implicit assertion isn't "any way a command could fail" - it's "things Cypress already checks about the subject". You never need to write ".should('exist')" about subjects, because Cypress implicitly asserts that they exist. In this case the subject is null
, and there are no implicit assertions that you could make explicit about it.
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.
Left some comments.
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Pushed up a reorganization of the I still have more feedback to address, but wanted to push up this much for people to look at. |
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 things:
1- can we update this PR title to reflect these changes.
2- I initially only looked at the doc preview since this PR has 111 file changes. with the restructuring of commands layout, I couldn't/still can't determine which commands are assertions or query commands when clicking through the list of commands. I didn't really realize the table of contents was trying to tell this story. Can we add a badge on the command pages to quickly highlight this?
@@ -246,7 +246,7 @@ by [Cypress Ambassador](https://www.cypress.io/ambassadors/) Josh Justice. | |||
## Log Cypress events | |||
|
|||
Cypress emits multiple events you can listen to as shown below. | |||
[Read more about logging events in the browser here](/api/events/catalog-of-events#Logging-All-Events). | |||
[Read more about logging events in the browser here](/api/cypress-api/catalog-of-events#Logging-All-Events). |
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.
The on-link slug needs updated to account for this url change. https://github.com/cypress-io/cypress-services/blob/develop/packages/on/data/links.yml#L518
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.
That on-link is pointing to a different sort of event, where developers meet up to discuss things. https://www.cypress.io/events/
The slug here looks to be correct to me?
@@ -411,7 +411,7 @@ to either: | |||
- Prevent Cypress from failing the test | |||
|
|||
This is documented in detail on the | |||
[Catalog Of Events](/api/events/catalog-of-events) page and the recipe | |||
[Catalog Of Events](/api/cypress-api/catalog-of-events) page and the recipe |
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.
Should this majority of these changes be a separate PR so these changes can go out now vs waiting for the v13 release?
It also would have been nice if this was its own PR to make this review a bit smaller.
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 think that it makes sense to roll it out all at once with Cy 13, so that we don't have to create multiple versions of the same changes (eg, create an "assertions" section of the new table of contents, then remove it in another PR on another branch). Reorganizing things piecemeal can result in a disjointed user experience; I'd rather have all the changes living in one branch so we can look at the organization holistically.
@@ -1,5 +1,6 @@ | |||
--- | |||
title: Catalog of Events | |||
sidebar_position: 10 |
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 would expect events to be its own document / api navigation section. I would not expect to find these under the API section.
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.
@@ -57,11 +57,28 @@ Pass in an options object to change the default behavior of `.uncheck()`. | |||
| `timeout` | [`defaultCommandTimeout`](/guides/references/configuration#Timeouts) | Time to wait for `.uncheck()` to resolve before [timing out](#Timeouts) | | |||
| `waitForAnimations` | [`waitForAnimations`](/guides/references/configuration#Actionability) | Whether to wait for elements to [finish animating](/guides/core-concepts/interacting-with-elements#Animations) before executing the command. | | |||
|
|||
### Requirements [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Chains-of-Commands) |
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 don't mind how this reads, but I do not believe the section Syntax
is the correct heading for these concepts since this is more than the syntax at this time.
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.
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 haven't applied them to every command yet because it'd involve manually moving around each file for a direction I'm not 100% sure on yet.
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.
Applied it to the first three commands: .and(), .as(), and .blur(), so people can see it 'live'. If we like the direction I'll go through and apply it to all commands.
### Yields [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Subject-Management) | ||
|
||
- `cy.window()` is a query command, and is |
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'd expect this to be the second bullet after we say what it yields
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.
So the reason I put the "is a query command" first is that some commands have long yields
sections. Eg:
I was aiming to always have it in the same place (eg, first or second), and in the cases where we need to spend time explaining what's yielded in more detail, having it second ended up making it feel buried under the other text.
I'd be fine breaking the consistency here, having it sometimes below what's actually yielded (you're right it is better in the short case
) and sometimes above (I do think it needs to come first in the 'long case'). Thoughts?
docs/api/commands/wait.mdx
Outdated
to go out. | ||
- When given an alias algument, `cy.wait()` can time out waiting for the | ||
response to return. |
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.
to go out. | |
- When given an alias algument, `cy.wait()` can time out waiting for the | |
response to return. | |
to go out or the | |
response to return. |
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 broke this out into two sentences because you can specify different request and response timeouts - it seemed like two different timeout options = two different ways it can time out.
|
||
### Assertions [<Icon name="question-circle"/>](/guides/core-concepts/introduction-to-cypress#Assertions) | ||
|
||
- Before executing, `.uncheck()` will wait for the element to reach an |
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.
- Before executing, `.uncheck()` will wait for the element to reach an | |
- Before executing, `.uncheck()` will wait for the subject to reach an |
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.
Why would we use a more generic term here? We know it's an element(s), rather than any generic "subject" (this is also detailed above as one of the 'requirements' of the command).
.should()
and .and()
are now queries.
Done.
A badge is possible, I'm just not sure the benefit. When visiting a command page, do you need to tell at a glance if a command is a query or not? I put it in the |
closing as this work is unplanned for the v13 release |
Matching code PR: cypress-io/cypress#25738
This PR reorganizes the API docs.
cy
.