cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
46.84k stars 3.17k forks source link

Wrong typings in cypress-npm-api.d.ts and cli/types/cypress.d.ts #23010

Open mirobo opened 2 years ago

mirobo commented 2 years ago

Current behavior

When using Cypress NPM API, i.e. cypress.run(..) or cypress.open(..) the parameter "specPattern" inside "config" does not exist. Yet it can be used and it actually works :-)

Please don't change that it works :-P

  const config = {
    project: absoluteTestPath,
    reporter: 'junit',
    headless: false,
    browser: 'electron',
    testingType: 'e2e',
    config: {
      specPattern: specList,
      video: false
    }
  };

The error was introduced here: https://github.com/cypress-io/cypress/commit/54f3b90b133f5dc15b407625f60a09eb62012214#diff-9fde1fb2f7eaba5ea49b585c811461495bfbba88e614e900a96863ecfe6b980aR2985

  /**
   * Config options that can be assigned on cypress.config.{ts|js} file
   */
  type UserConfigOptions<ComponentDevServerOpts = any> = Omit<ResolvedConfigOptions<ComponentDevServerOpts>, 'baseUrl' | 'excludeSpecPattern' | 'supportFile' | 'specPattern'>

Not quite sure why the UserConfigOptions are omitting excludeSpecPattern and specPattern (personally I don't care about supportFile, but if it works, it should not be omitted). Same goes for baseUrl.

I think it would be helpful to A) clean wrong omissions and maybe don't maintain two "sources of truth" for which parameters are possible: https://docs.cypress.io/guides/guides/module-api

I'd rather like to see just an automatically generated list of options, derived from the type UserConfigOptions ? But this is probably something cypress-documentation should think about :D. If the type is correct I'm happy but the documentation should be up to date.

Desired behavior

Fixed types

Test code to reproduce

n/a

Cypress Version

10.3.1

Other

No response

tbiethman commented 2 years ago

@mirobo thanks for the ticket. I believe the current typing indicates our desired usage, which would be to nest the specPattern under an associated e2e or component key:

  const config = {
    project: absoluteTestPath,
    reporter: 'junit',
    headless: false,
    browser: 'electron',
    testingType: 'e2e',
    config: {
      e2e: {
        specPattern: specList,
      },
      video: false
    }
  };

It's a little verbose, as you're already specifying the testingType separately, but the config types and config docs do communicate a consistent API. I don't see any mention of specPattern being supported directly in the module api docs. Let me know if I'm overlooking something or if something could be made clearer.

mirobo commented 2 years ago

Thanks for the hint. But I wonder why it works if it's wrong. I fixed it in my project. But I've found other stuff:

config.e2e shows these options (and they work): image

but the documentation shows only couple of options: https://docs.cypress.io/guides/references/configuration#e2e image

1) Is it immediately clear based on the documentation that the base options are also configurable inside "e2e"?

2) slowTestThreshold can be configured in both places (but the documentation doesn't mention this as a base configuration)

image

3) Also the option "modifyObstructiveCode" disappeared from the type? It's wrong inside e2e and also inside config..

image image

tbiethman commented 2 years ago

@mirobo ah yeah, I can confirm your findings here.

1 and 2: I believe some typings are being reused here without the expected omits, which is very confusing. Especially so because as you said the values work in both locations. We can do better here.

3: It does appear modifyObstructiveCode has been incorrectly removed from the config type, we will get that fixed.

mirobo commented 2 years ago

Point 3 is already adressed here #22146

jennifer-shehane commented 1 year ago

Is this still an issue in 13.x versions? We made some updates to typings.

mirobo commented 1 year ago

Is this still an issue in 13.x versions? We made some updates to typings.

I didn't check it yet. I'm closing the issue and I'd reopen a new one if still relevant. First we'll need to check if the Cypress Gateway Connector still let's us use sorry-cypress with 13.x (I know it should!).

On a side note: Sad that it had to come to this: https://currents.dev/posts/v13-blocking because we need an on-premise solution and not getting future upgrades might (in long term) just lead us back to some simple parallel test execution by splitting tests based on the number of runners. This won't pay for the future development of Cypress and I hope that there will be maybe an initiative to fund it's development with tiny voluntary support payments by the users themselves? I personally think that the Cypress - the company - deserves that and I still very much prefer Cypress over Playwright, no matter how many people tell "I'm switching to Playwright". I wonder why Google or Meta doesn't publicly show interest in the tool, I think it would nice if they also support it :-)