chore: Address skipped specs in server package #22356

astone123 · 2022-06-16T13:48:58Z

Closes Audit and address spec skips in server package related to unification dev #21966

User facing changelog

n/a

Additional details

Some test suites were skipped during the unification effort. We need to re-visit those skipped tests and either revise them to cover new workflows, get coverage elsewhere and/or remove them.

I marked the diff with explanations for the changes that I made. Look through the changes and make sure that you agree with what changes were made to the tests.

I revised and/or deleted most single tests that were skipped, but there were some entire test files that were skipped and I think that we should handle those in separate tickets. You can see all of the related tickets organized in this epic

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-06-16T13:49:01Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

astone123 · 2022-06-16T13:49:56Z

packages/server/test/integration/cypress_spec.js

@@ -1731,26 +1702,6 @@ describe('lib/cypress', () => {
      })
    })

-    // NOTE: skipped because we want to ensure this is captured in v10


This workflow is covered by a system test system-tests/test/network_error_handling_spec.js

We also have it here for launchpad:

https://github.com/cypress-io/cypress/blob/develop/packages/launchpad/cypress/e2e/config-files-error-handling.cy.ts#L242-L271

…ipped-server-specs

astone123 · 2022-06-17T14:52:31Z

packages/server/test/unit/browsers/firefox_spec.ts

-    // TODO: pick open port for debugger
-    it.skip('finds remote port for firefox debugger', function () {
-      return firefox.open(this.browser, 'http://', this.options, this.automation).then(() => {
-        // expect(firefoxUtil.findRemotePort).to.be.called


findRemotePort doesn't exist in firefoxUtil anymore

astone123 · 2022-06-17T14:52:49Z

packages/server/test/unit/config_spec.js

-      })
-
-      // NOTE: Validated in real use
-      it.skip('validates cypress.env.json', function () {


Added a system test for cypress.env.json validation

astone123 · 2022-06-17T14:53:46Z

packages/server/test/unit/config_spec.js

@@ -1481,24 +1472,26 @@ describe('lib/config', () => {

        return config.mergeDefaults(obj, options)
        .then((cfg) => {
+          ['platform', 'arch'].forEach((x) => {


platform and arch will be different in CI, so check that they exist but remove them from the config before asserting equality

cypress · 2022-06-17T15:15:00Z

Test summary

37681 • 0 • 456 • 0 • 9

Run details


Project	cypress
Status	Passed
Commit	`b6d1df2`
Started	Jun 24, 2022 4:34 PM
Ended	Jun 24, 2022 4:52 PM
Duration	18:47 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/actions/click.cy.js	1
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	commands/xhr.cy.js	2
1	... > logs request + response headers
2	... > logs response
	cypress/proxy-logging.cy.ts	2
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged first
2	... > intercept log has consoleProps with intercept info
This comment includes only the first 5 flaky tests. See all 9 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

astone123 · 2022-06-17T19:15:02Z

packages/server/test/unit/config_spec.js

@@ -578,27 +570,27 @@ describe('lib/config', () => {
      })

      // TODO:(lachlan): after mega PR
-      context.skip('specPattern', () => {
+      context('specPattern', () => {


Just want to point out that fixing this test uncovered that we aren't validating the specPattern property in our latest version. This would be an issue if a user had a specPattern in their config that wasn't either a string or an array of strings. For example, if I had

specPattern: 5

in my configuration, I would see this error

now, I would see this error

Lol, as soon as we skipped a test, the functionality regressed and no-one noticed. Obvious lesson is don't skip tests!!!!! 🤦

astone123 · 2022-06-21T18:20:22Z

packages/server/test/unit/gui/events_spec.js

-const errors = require(`../../../lib/errors`)
-const browsers = require(`../../../lib/browsers`)
-const { openProject } = require('../../../lib/open_project')
-const events = require(`../../../lib/gui/events`)


I deleted this test because the functions exported by the events module don't appear to do anything. The body of each condition of the switch statement is commented out.

Let's make a ticket to clean those out, iirc this is stuff @tgriesser kept around for reference purposes during a transition. Doesn't quite make sense to remove them in this PR for sure. But maybe Tim can tell us if it's safe to remove now.

marktnoonan

Some good changes here! Requesting one change in the validation, to include the default specPattern values. I might have one or two other questions tomorrow.

For the tests you mentioned in the description that aren't addressed in this PR, let's make those issues and link them up before merging the PR. It might be good to reach out to @tgriesser and get some notes on what is needed to fix them, or confirm if, say, these unit tests are still needed, given that the comment says this is tested through open mode now.

marktnoonan · 2022-06-22T02:44:04Z

packages/config/src/options.ts

@@ -419,6 +419,11 @@ const resolvedOptions: Array<ResolvedConfigOption> = [
    canUpdateDuringTestTime: false,
    requireRestartOnChange: 'server',
  },
+  {
+    name: 'specPattern',


This is a good catch! We seem to also declare the default values in other cases. Looks like we can use the same options.testingType ternary that's being used for viewportWidth etc to populate the defaults that are declared in this file already in the defaultSpecPattern object. What do you think?

I added that but then I just removed it again... I'm confused about what defaultValue does here. It looks like it was messing up some of the validation in the tests when I had it in there 😕

Let's make an issue and add a comment for this then, totally worth investigate.

Added an issue and a comment here

lmiller1990

Some comments

lmiller1990 · 2022-06-23T05:32:35Z

packages/config/src/options.ts

+    defaultValue: (options: Record<string, any> = {}) => options.testingType === 'component' ? defaultSpecPattern.e2e : undefined,
+    validation: validate.isStringOrArrayOfStrings,
+    canUpdateDuringTestTime: false,
+  },


Am I missing something, this seems the opposite of what we want:

options.testingType === 'component' ? defaultSpecPattern.e2e

If we are using component testing, default to the e2e spec pattern?

I think you want these:

cypress/packages/config/src/options.ts

Lines 109 to 112 in 50cc582

export const defaultSpecPattern = {

e2e: 'cypress/e2e/**/*.cy.{js,jsx,ts,tsx}',

component: '**/*.cy.{js,jsx,ts,tsx}',

}

Maybe it should be

defaultValue: (options: Record<string, any> = {}) =>defaultSpecPattern[options.testingType],

Yeah that was wrong... I ended up removing the defaultValue because it was causing issues. It seems like options.testingType was undefined sometimes when inside of the defaultValue function. I don't think that we need that property here based on the tests

lmiller1990 · 2022-06-23T05:36:15Z

packages/server/test/integration/cypress_spec.js

@@ -1731,26 +1702,6 @@ describe('lib/cypress', () => {
      })
    })

-    // NOTE: skipped because we want to ensure this is captured in v10


We also have it here for launchpad:

https://github.com/cypress-io/cypress/blob/develop/packages/launchpad/cypress/e2e/config-files-error-handling.cy.ts#L242-L271

lmiller1990 · 2022-06-23T05:37:35Z

packages/server/test/unit/config_spec.js

@@ -1525,7 +1518,6 @@ describe('lib/config', () => {
            supportFile: { value: false, from: 'config' },
            supportFolder: { value: false, from: 'default' },
            taskTimeout: { value: 60000, from: 'default' },
-            specPattern: { value: '**/*.*', from: 'default' },


Why did this get removed?

packages/server/test/unit/config_spec.js

lmiller1990 · 2022-06-23T05:42:30Z

As for adding missing coverage, I'd agree we make new tickets for each one (or maybe can group some). Would you like to to that? You could then reference this one in their description.

I think

Are probably the more important ones (and likely can be replaced by a handful of integration tests, instead of 50+ unit tests).

lmiller1990

Looks good, my changes have been incorporated, or spawned new issues.

…ipped-server-specs

…press into 21966-skipped-server-specs

tbiethman · 2022-06-24T14:17:57Z

packages/server/test/unit/config_spec.js

@@ -578,27 +572,27 @@ describe('lib/config', () => {
      })

      // TODO:(lachlan): after mega PR


The TODO has been addressed, should be safe to remove at this point.

Requesting review again

lmiller1990 · 2022-06-27T02:34:23Z

I do no know what's going on with the failing windows test, but it's happening in multiple branches, and I do not think it was introduced here. I am going to force merge this, then look into the windows CI fail.

@marktnoonan

…esser/CLOUD-577-spec-list-display-latest-runs-batching * muaz/CLOUD-577-spec-list-display-latest-runs: test: Addressing launchpad test flake in Windows (#22536) address comments from @marktnoonan Address code review comments followup on other comments re: @lmiller1990 PR comments chore(deps): update dependency semantic-release to v19 [security] (#22238) chore: Address skipped specs in server package (#22356) Address code review findings Address code review findings Empty-Commit to generate new percy nonce fix: handle case of implicit plugins/index.js files during migration (#22501)

chore: Un-skip some tests

356fbde

astone123 self-assigned this Jun 16, 2022

astone123 commented Jun 16, 2022

View reviewed changes

astone123 added 3 commits June 17, 2022 10:30

Merge branch 'develop' of github.com:cypress-io/cypress into 21966-sk…

ee6d2da

…ipped-server-specs

chore: Unskip more tests

3239348

chore: Use exec rather than spawn

4ae8a31

astone123 commented Jun 17, 2022

View reviewed changes

chore: Fix test

4f82a91

astone123 commented Jun 17, 2022

View reviewed changes

chore: Remove events_spec tests

589a82e

astone123 commented Jun 21, 2022

View reviewed changes

Merge branch 'develop' into 21966-skipped-server-specs

eb3666d

astone123 marked this pull request as ready for review June 21, 2022 18:42

astone123 requested review from a team as code owners June 21, 2022 18:42

astone123 requested review from chrisbreiding and removed request for a team June 21, 2022 18:42

marktnoonan self-requested a review June 22, 2022 02:33

marktnoonan previously requested changes Jun 22, 2022

View reviewed changes

astone123 requested review from tgriesser, lmiller1990, ZachJW34 and tbiethman June 22, 2022 16:54

chore: Add defaultValue for specPattern option

50cc582

lmiller1990 reviewed Jun 23, 2022

View reviewed changes

astone123 added 2 commits June 23, 2022 11:36

chore: Remove default value

cafd029

chore: Use os to determine system arch and platform during testing

25085da

astone123 requested a review from marktnoonan June 23, 2022 17:55

Merge branch 'develop' into 21966-skipped-server-specs

6d41946

lmiller1990 self-requested a review June 24, 2022 00:53

lmiller1990 approved these changes Jun 24, 2022

View reviewed changes

astone123 added 2 commits June 24, 2022 09:37

Merge branch 'develop' of github.com:cypress-io/cypress into 21966-sk…

31a837d

…ipped-server-specs

Merge branch '21966-skipped-server-specs' of github.com:cypress-io/cy…

e0f17a4

…press into 21966-skipped-server-specs

astone123 mentioned this pull request Jun 24, 2022

Figure out whether specPattern config option should have default value #22507

Closed

chore: Add comment for specPattern defaultValue

3019d1f

tbiethman reviewed Jun 24, 2022

View reviewed changes

tbiethman approved these changes Jun 24, 2022

View reviewed changes

chore: Remove comment

f32c6cd

astone123 added 2 commits June 24, 2022 10:49

Merge branch 'develop' into 21966-skipped-server-specs

93ad270

Merge branch 'develop' into 21966-skipped-server-specs

b6d1df2

marktnoonan approved these changes Jun 25, 2022

View reviewed changes

lmiller1990 merged commit 22d2c31 into develop Jun 27, 2022

lmiller1990 deleted the 21966-skipped-server-specs branch June 27, 2022 02:34

cypress-bot bot mentioned this pull request Jun 27, 2022

Audit and address spec skips in server package related to unification dev #21966

Closed

lmiller1990 mentioned this pull request Dec 19, 2022

Cypress is running really slow after upgrading to v10 #22868

Closed

	export const defaultSpecPattern = {
	e2e: 'cypress/e2e/*/.cy.{js,jsx,ts,tsx}',
	component: '*/.cy.{js,jsx,ts,tsx}',
	}

		@@ -578,27 +572,27 @@ describe('lib/config', () => {
		})

		// TODO:(lachlan): after mega PR

chore: Address skipped specs in server package #22356

chore: Address skipped specs in server package #22356

Uh oh!

Conversation

astone123 commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User facing changelog

Additional details

PR Tasks

Uh oh!

cypress-bot bot commented Jun 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cypress bot commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Run details

Flakiness

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marktnoonan Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marktnoonan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmiller1990 commented Jun 23, 2022

Uh oh!

lmiller1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmiller1990 commented Jun 27, 2022

Uh oh!

Uh oh!

astone123 commented Jun 16, 2022 •

edited

Loading

cypress bot commented Jun 17, 2022 •

edited

Loading

marktnoonan Jun 23, 2022 •

edited

Loading