add Loggable options to its() / invoke() command

[gabbersepp]

• Closes The its command doesn't accept a log option (logging can't be disabled) #1450

User facing changelog

The commands .its() and .invoke() now accept an options object of type Loggable to indicate that logging should be skipped.

Additional details

This change does not break any existing implementations. By default logging is enabled.

How has the user experience changed?

its Before

cy.get('.connectors-its-ul>li')
  .its('length')
  .should('be.gt', 2)

its After

cy.get('.connectors-its-ul>li')
  .its('length', { log: false }) // no log
  .should('be.gt', 2)

invoke Before

cy.get('.connectors-div').should('be.hidden')
  .invoke('show') // will always log
  .should('be.visible')

invoke After

cy.get('.connectors-div').should('be.hidden')
  .invoke({ log: false }, 'show') // no log
  .should('be.visible')

PR Tasks

• Have tests been added/updated?
• Has a PR for user-facing changes been opened in cypress-documentation? add docu for new options argument of its() cypress-documentation#2205
• Have API changes been updated in the type definitions?

Edited by @jennifer-shehane to add before and after headings for its and invoke + add invoke change to changelog.

[cypress-bot]

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

• Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
• Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

• The feature/bugfix is self-documenting from within the product.
• The change provides the end user with a way to fix their problem (no dead ends).

Functionality

• The code works and performs its intended function with the correct logic.
• Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
• The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

• The code is readable (too many nested 'if's are a bad sign).
• Names used for variables, methods, etc, clearly describe their function.
• The code is easy to understood and there are relevant comments explaining.
• New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
• There are comments containing link(s) to the addressed issue (in tests and code).

Quality

• The change does not reimplement code.
• There's not a module from the ecosystem that should be used instead.
• There is no redundant or duplicate code.
• There are no irrelevant comments left in the code.
• Tests are testing the code’s intended functionality in the best way possible.

Internal

• The original issue has been tagged with a release in ZenHub.

[jennifer-shehane]

Hey @gabbersepp, I see some tests failing around the its command in our CI here: https://circleci.com/gh/cypress-io/cypress/177593

If there is still work being done, add the WIP to the title of the PR so that we don't review the changes before they're done. Remove this from the title when you're ready for review.

Let us know if you have any questions we can help with in the meantime!

[gabbersepp]

Hey @gabbersepp, I see some tests failing around the its command in our CI here: https://circleci.com/gh/cypress-io/cypress/177593

If there is still work being done, add the WIP to the title of the PR so that we don't review the changes before they're done. Remove this from the title when you're ready for review.

Let us know if you have any questions we can help with in the meantime!

Hey,
sorry did not see the failing tests.

I don't understand how the "logs immediately before resolving" can fail.
Maybe another one can have a look at it.

What I have done and what I do not understand how this can work:

• set a breakpoint in connectors.coffee at line 175
• set a breakpoint in connectors_spec.coffee at line 892
• if the test is executed, try to step over line 175 and into the call to set() in line 176
• the code is immediatelly jumping into the breakpoint at line 892 with all the information set ("its", "foo", ....)

How can this happen? Line 175 only calls Cypress.Log() without passing anything but nevertheless line 892 is called?

@jennifer-shehane maybe you can give me a hint here.

[gabbersepp]

Btw if I use following test instead:

 it "logs immediately before resolving", (done) ->
          #cy.on "log:added", (attrs, log) ->
          #  if log.get("name") is "its"
          #    expect(log.get("state")).to.eq("pending")
          #    expect(log.get("message")).to.eq ".foo"
          #    done()

          cy.noop({foo: "foo"}).its("foo").then ->
            lastLog = @lastLog
            expect(lastLog.get("message")).to.eq(".foo")
            done()

everything is green

//Edit:

Seems strange:

Both, the log received in the event and the log in lastLog are the same but the message differs.
ll.get("message") evaluates to .foo so the message of the log entry is manipulated afterwards. No idea where this happens

[gabbersepp]

ok. finally fixed the test. :-)

[jennifer-shehane]

Great. Sorry we couldn't get any eyes on this sooner to help.

[jennifer-shehane]

I tested this manually and it worked when used as designed.

I fould a few error edge cases that need better handling.

1. If someone were to call .invoke({ log: false }) and forget the second argument an error is thrown that would not be clear to the user what they did wrong.

   Can you add special error handling for this case with a message informing that they need to pass in a function argument and add a test for this also?

   

2. If I pass a non-String as the 2nd argument after passing the options Object .invoke({ log: false }, 13), the error message incorrectly tells me that the 1st argument only accepts String, it should say 'Second'. Can you add a test for this also?

   

3. If I pass a non-Object to its options argument .its('length', 'foo') the function of its no longer functions and returns undefined. This should throw an error saying we only accept an options Object as the second argument - add a test for this.

   

4. Passing a non-Object or String as the first argument to invokes causes unexpected results when continuing to chain the return value of the invoke .invoke(13, 'show'). We should throw if the first argument is not an Object or String - add a test for this also.

   

[gabbersepp]

Thanks. There where some bugs in it. Hopefully I fixed all of them.
And I added some more tests.

Personally I would prefer to split up this spec file into several smaller ones as now the file has about 1500 lines of code.

[bahmutov]

need to resolve merge conflicts first

[gabbersepp]

@bahmutov
done. We will see in a few minutes, if everything is green.

But after reading through #5755 I am not sure if the changes made on invoke in this PR should be merged, because @flotwig mentioned, that he expect[s] invoke to have the same function signature as an its call. This does not hold anymore now as invoke accepts the options as first parameter to not conflict with potential passed function arguments.

[flotwig]

@bahmutov
done. We will see in a few minutes, if everything is green.

But after reading through #5755 I am not sure if the changes made on invoke in this PR should be merged, because @flotwig mentioned, that he expect[s] invoke to have the same function signature as an its call. This does not hold anymore now as invoke accepts the options as first parameter to not conflict with potential passed function arguments.

It will make the contract a little weird, but the way you have it here is really the only way I can see to do it. 👍 on this change.

[jennifer-shehane]

I had a minor editing to the wording I added. Otherwise this looks good.

merge conflicts fixed